-
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(remotes): add remotes and configs #609
feat(remotes): add remotes and configs #609
Conversation
Wow, that's an amazing work! Thank you a lot! I will take a look at the PR this week and come back with the feedback. |
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.
Amazing work! Thank you for the PR. I have fixed a few places that I considered important. I think the PR is pretty done. I will take a look at it one more time a bit later and merge it.
The only concern for me is merging extends for different remotes. I will try to figure out how to fix it (if I reproduce this issue of course)
if err = extend(v, filepath.Dir(configPath)); err != nil { | ||
return 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.
Looks like we will extend the same things multiple times, and if files from different remotes have extends
it will break here.
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.
Ok, I've tested this concern and I coundn't reproduce an error, because extends
get replaced after merging each remote. That's fine. I will polish the PR a little bit and prepare it for the release.
internal/lefthook/run.go
Outdated
if cfg.Remote != nil { | ||
cfg.Remotes = append(cfg.Remotes, cfg.Remote) | ||
} |
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 can be done in load.go
Closes # (issue)
No issues closed by this, but a complement of : #343 /
#420
⚡ Summary
Evolution of remote configuration, enabling the integration of multiple configurations from one or more different repositories.
Example :
I have retained the old functionality with a single remote and a single config, which is still operational (but deprecated).
I needed to add an exclusion of gocyclo for the TestLoad function (internal/config/load_test.go) because it was reporting excessively high complexity. However, it doesn't seem critical since it's an addition of a test for the new feature.
Additionally, I tried to add deprecation messages for those still using 'remote' and 'config,' but it breaks the tests. Do you have a solution to avoid this?
I hope this work suits you well; feel free to provide feedback if needed!
☑️ Checklist