Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: configure hook remote repositories #323

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ require (
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/subosito/gotenv v1.3.0 // indirect
golang.org/x/sync v0.0.0-20220819030929-7fc1605a5dde
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
golang.org/x/text v0.3.7 // indirect
gopkg.in/ini.v1 v1.66.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ
golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20220819030929-7fc1605a5dde h1:ejfdSekXMDxDLbRrJMwUk6KnSLZ2McaUCVcIKM+N6jc=
golang.org/x/sync v0.0.0-20220819030929-7fc1605a5dde/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
Expand Down
7 changes: 7 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
type Config struct {
Colors bool `mapstructure:"colors"`
Extends []string `mapstructure:"extends"`
Remotes []Remote `mapstructure:"remotes"`
MinVersion string `mapstructure:"min_version"`
SkipOutput []string `mapstructure:"skip_output"`
SourceDir string `mapstructure:"source_dir"`
Expand All @@ -15,6 +16,12 @@ type Config struct {
Hooks map[string]*Hook
}

type Remote struct {
URL string `mapstructure:"url"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think url option is too general. Maybe git?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would git_url or git_repo be an acceptable option? In #155 the posted example uses repo which may be another good alternative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like git_url, it perfectly explains the meaning of an option.

Rev string `mapstructure:"rev"`
Configs []string `mapstructure:"configs"`
}

func (c *Config) Validate() error {
return version.CheckCovered(c.MinVersion)
}
100 changes: 91 additions & 9 deletions internal/config/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,15 @@ package config
import (
"path/filepath"
"regexp"
"sort"
"strings"
"sync"

"github.com/evilmartians/lefthook/internal/git"
"github.com/evilmartians/lefthook/internal/log"
Comment on lines +10 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think it's better to devide merging configs and installing the remotes. It's better to put this logic into internal/lefthook/install.go

"github.com/spf13/afero"
"github.com/spf13/viper"
"golang.org/x/sync/errgroup"
)

const (
Expand All @@ -24,7 +29,7 @@ func Load(fs afero.Fs, path string) (*Config, error) {
return nil, err
}

extends, err := mergeAllExtends(fs, path)
extends, err := mergeAll(fs, path)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -61,13 +66,17 @@ func read(fs afero.Fs, path string, name string) (*viper.Viper, error) {
return v, nil
}

// Merges extends from .lefthook and .lefthook-local.
func mergeAllExtends(fs afero.Fs, path string) (*viper.Viper, error) {
// mergeAll merges remotes and extends from .lefthook and .lefthook-local.
func mergeAll(fs afero.Fs, path string) (*viper.Viper, error) {
extends, err := read(fs, path, "lefthook")
if err != nil {
return nil, err
}

if err := remotes(fs, extends); err != nil {
return nil, err
}

if err := extend(fs, extends); err != nil {
return nil, err
}
Expand All @@ -79,26 +88,99 @@ func mergeAllExtends(fs afero.Fs, path string) (*viper.Viper, error) {
}
}

if err := remotes(fs, extends); err != nil {
return nil, err
}

Comment on lines +91 to +94
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to run remotes one more time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I hadn't added the option to specify the config to use, so it first searched for lefthook.yml inside of the remote and then lefthook-local.yml. Thinking on it now, I don't think this made sense even then since lefthook-local.yml should not be commited. 😅 I'll go ahead and remove it when moving this over to internal/lefthook/install.go.

if err := extend(fs, extends); err != nil {
return nil, err
}

return extends, nil
}

func extend(fs afero.Fs, v *viper.Viper) error {
for _, path := range v.GetStringSlice("extends") {
name := strings.TrimSuffix(filepath.Base(path), filepath.Ext(path))
func remotes(fs afero.Fs, v *viper.Viper) error {
var remotes []Remote
err := v.UnmarshalKey("remotes", &remotes)
if err != nil {
return err
}

another, err := read(fs, filepath.Dir(path), name)
if err != nil {
if len(remotes) == 0 {
return nil
}

var (
wg sync.WaitGroup
eg errgroup.Group
configPaths []string
configPathCh = make(chan string)
)

wg.Add(1)
go func() {
for configPath := range configPathCh {
configPaths = append(configPaths, configPath)
}
wg.Done()
}()

for i := range remotes {
remote := remotes[i]
eg.Go(func() error {
dir, err := git.InitRemote(fs, remote.URL, remote.Rev)
if err != nil {
return err
}

for _, path := range remote.Configs {
configPathCh <- filepath.Join(dir, path)
}
return nil
})
}

// Wait on errgroup to finish before closing the channel.
err = eg.Wait()
close(configPathCh)
if err != nil {
return err
}

// Wait for all of the configPaths to be added.
wg.Wait()

// Stable sort to ensure that the merge order is deterministic.
sort.SliceStable(configPaths, func(i, j int) bool { return configPaths[i] < configPaths[j] })

for _, configPath := range configPaths {
log.Debugf("Merging remote config: %v", configPath)
if err := merge(fs, configPath, v); err != nil {
return err
}
if err = v.MergeConfigMap(another.AllSettings()); err != nil {
}
return nil
}

func extend(fs afero.Fs, v *viper.Viper) error {
for _, path := range v.GetStringSlice("extends") {
if err := merge(fs, path, v); err != nil {
return err
}
}
return nil
}

func merge(fs afero.Fs, path string, v *viper.Viper) error {
name := strings.TrimSuffix(filepath.Base(path), filepath.Ext(path))

another, err := read(fs, filepath.Dir(path), name)
if err != nil {
return err
}
if err = v.MergeConfigMap(another.AllSettings()); err != nil {
return err
}
return nil
}

Expand Down
80 changes: 80 additions & 0 deletions internal/git/remote.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package git

import (
"errors"
"os"
"path/filepath"
"strings"

"github.com/evilmartians/lefthook/internal/log"
"github.com/spf13/afero"
)

var defaultRemotePath = mustGetDefaultRemotesDir()

// mustGetDefaultRemotesDir returns the default directory for the lefthook remotes.
func mustGetDefaultRemotesDir() string {
homeDir, err := os.UserHomeDir()
if err != nil {
panic(err)
}
return filepath.Join(homeDir, ".lefthook-remotes")
}
Comment on lines +16 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would really surprise users with new directories. I think it's better to put remote config in .git/info directory. This would also save us from name conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! I wasn't aware that the .git/info directory could be used in this manner. Thanks for mentioning this 🙇🏽


// InitRemote clones or pulls the latest changes for a git repository that was specified as
// a remote config repository. If successful, the path to the root of the repository will be
// returned.
func InitRemote(fs afero.Fs, url, rev string) (string, error) {
err := fs.MkdirAll(defaultRemotePath, 0755)
if err != nil && !errors.Is(err, os.ErrExist) {
return "", err
}

root := getRemoteDir(url)

_, err = fs.Stat(root)
if err == nil {
if err := updateRemote(fs, root, url, rev); err != nil {
return "", err
}
return root, nil
}

if err := cloneRemote(fs, root, url, rev); err != nil {
return "", err
}
return root, nil
}
Comment on lines +27 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can be a part of lefthook install command, and it shouldn't pull the remote config if it already exists. lefthook install --force can be used to force that. Or we can have another option, like --sync-remotes/-s 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of having the --sync-remotes/-s flags re-sync the remote config. An alternative short version could be -S to avoid reserving -s. This could be useful later on since -s is commonly used as an alternative to --silent (althought LEFTHOOK=0 already accomplishes this so it is most likely unnecessary).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to use either -S or -s. Both variants work for me :)


func updateRemote(fs afero.Fs, root, url, rev string) error {
log.Debugf("Updating remote config repository: %v", root)
cmdFetch := []string{"git", "-C", root, "pull", "-q"}
if len(rev) == 0 {
cmdFetch = append(cmdFetch, "origin", rev)
}
_, err := execGit(strings.Join(cmdFetch, " "))
if err != nil {
return err
}
return nil
}

func cloneRemote(fs afero.Fs, root, url, rev string) error {
log.Debugf("Cloning remote config repository: %v", root)
cmdClone := []string{"git", "-C", defaultRemotePath, "clone", "-q"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use long names for git options just to be more verbose and obvious.

Comment on lines +62 to +64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function makes sense 👍

I just think we don't need to store the whole repo. We can clone it into under a /tmp, take the desired file, and drop it. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good to me!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use shallow clone as well to further cut back on larger repos.

if len(rev) > 0 {
cmdClone = append(cmdClone, "-b", rev)
}
cmdClone = append(cmdClone, url)
_, err := execGit(strings.Join(cmdClone, " "))
if err != nil {
return err
}
return nil
}

func getRemoteDir(url string) string {
// Removes any suffix that might have been used in the url like '.git'.
trimmedURL := strings.TrimSuffix(url, filepath.Ext(url))
return filepath.Join(defaultRemotePath, filepath.Base(trimmedURL))
}