-
Notifications
You must be signed in to change notification settings - Fork 220
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
Conversation
Signed-off-by: Oscar Alberto Tovar <oscar.alberto.tovar@gmail.com>
Hey, thank you a lot! I will take a look tomorrow or in 1-2 days! |
Signed-off-by: Oscar Alberto Tovar <oscar.alberto.tovar@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! This PR totally makes sense! Thank you for contribution! I have left a few comments here.
I also have a general question: why allowing many configs instead of one config? I am not sure many people even use .lefthook-local.yml
. I can imagine a use-case where you have, for example, pre-commit-lefthook.yml
and pre-push-lefthook.yml
, and you want them to be used separately or together in different cases. But I think it makes things a bit more complicated. You can have just different files with all the hooks you want to have. I would like to start with just config
or config_name
option (which can be set to lefthook.yml
by default).
The same is for remotes
. I think we can start with a single remote
option.
If you have a use-case where multiple remotes and/or configs are useful, we can discuss it. I just want to keep things simple as much as possible.
The questions I want to sum up:
- WDYT of having single
remote
andconfig
options? Can we start with that? - Let's put the downloading logic (plus checking if installing is needed) into
install
command? - Let's keep remote config under
.git/info
directory? - Let's add some unit tests?
You did a great job! I think this is a really missing feature of lefthook!
"github.com/evilmartians/lefthook/internal/git" | ||
"github.com/evilmartians/lefthook/internal/log" |
There was a problem hiding this comment.
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
if err := remotes(fs, extends); err != nil { | ||
return nil, err | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
func mustGetDefaultRemotesDir() string { | ||
homeDir, err := os.UserHomeDir() | ||
if err != nil { | ||
panic(err) | ||
} | ||
return filepath.Join(homeDir, ".lefthook-remotes") | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙇🏽
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 | ||
} |
There was a problem hiding this comment.
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
🤔
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 cloneRemote(fs afero.Fs, root, url, rev string) error { | ||
log.Debugf("Cloning remote config repository: %v", root) | ||
cmdClone := []string{"git", "-C", defaultRemotePath, "clone", "-q"} |
There was a problem hiding this comment.
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.
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"} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
@@ -15,6 +16,12 @@ type Config struct { | |||
Hooks map[string]*Hook | |||
} | |||
|
|||
type Remote struct { | |||
URL string `mapstructure:"url"` |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@mrexox Thanks for the valuable feedback! I've responded inside of the threads, but I'll summarize here as well. I did not have much time to work on this today, but I will update my branch with the provided feedback tomorrow and push. Thanks again!
Absolutely! This would make it easier to test and easier to deliver 👍🏽
👍🏽 I agree with this. One thing that I was worried about was the constant fetching. Fetching the git repo only on install would reduce the chance that this puts too much of a strain on the git server.
Yes, this would prevent new directories from being created and also take care of an edge case. If there are two repos,
😄 Definitely, I will add some test cases for the refactored functionality. |
Hey @oatovar! It's been quite a while since we worked on this feature. If it is not convenient for you to continue, I can catch it and proceed. Don't you mind? |
@mrexox Sure thing! Sorry, I've had a lot on my plate recently, so I haven't been able to make progress on it. |
Summary
This PR adds a feature to utilize shared hooks as explained in #155. It works by utilizing a
.lefthook-remotes
directory inside of the home directory to store all of the git repositories. If the repository exists, then it will pull changes instead of cloning a new repository.Defining them looks like the following:
The new order of config merges is as follows.
Resolves #155