Closed ardelato closed 1 month ago
Hey! Thank you for the issue. I have prepared the fix: https://github.com/evilmartians/lefthook/pull/754. Now extends
option will be merged for the local config only if it differs from global extends.
Please, check out 1.6.17 for the fix
@mrexox That fix worked out!
But oh man I didn't expect this turn around speed! Thank you very much @mrexox š
@mrexox I am sorry to say but the recent fix may have exposed a different bug.
The bug may just be our fault for how we implemented the lefthook
configs.
For context, we currently have a lefthook-config
package which neatly houses all our commands and scripts. We were partly inspired by Nozomiishii's configuration approach here - https://github.com/nozomiishii/configs/tree/592d241fdc5fb6750b03fbca9e95c8f15e31e9f5/packages/lefthook-config
The package's lefthook.yml
file extends
all the commands. We then extend
this package's lefthook.yml
in our global lefthook.yml
. Overall, we have a set of nested extends
at least 2 levels deep.
The issue is that the second level extends
is no longer applied. Before the change, the code would still enter this block, even if there was no lefthook-local.yml
file, and thus execute the extends
function. As such, the second level extend commands would be applied.
Overall, was lefthook
's extend
option meant to be nested like this?
@ardelato , thank you for providing a great example of the configuration setup! I never thought of such cases with multiple extends
levels. Originally it was supposed to have only one level of extends. I think it might be useful to allow as many levels as possible but it may require some time to implement this feature.
If you used to share the configuration I can suggest you the lefthook's remotes
feature. You can specify multiple configs for a single remote which might cover your use case.
I will try to implement recursive merging of extends but I think it may take some time. I'll let you know if I find a blocker.
Hey @ardelato! It was easier than I thought. I will prepare a new version with this fix soon
Hot damn! I would have been just fine if this just went on your radar but to address it so quickly makes me really appreciate this repo and the work you guys are doing š.
That said, the new version does seem to have fixed both issues now š. Thank you @mrexox.
:wrench: Summary
We recently came across some unexpected behavior when trying to make use of the
exclude_tags
option in alefthook-local.yml
.We had already declared a
exclude_tags
option in one of theextends
files included through the global config, but we wanted to overwrite it through alefthook-local.yml
config.We discovered that the
extends
files will take precedence over thelefthook-local.yml
config because it gets re-applied at this line: https://github.com/evilmartians/lefthook/blob/8959135e2f4da83b44edde075bd16d581492f0c2/internal/config/load.go#L116-L117Steps to reproduce
I added a test to
load_test.go
to reproduce the issue: https://github.com/evilmartians/lefthook/compare/master...ardelato:bug-repro--extends-takes-precendence-over-localExpected results
I expected all options from the
lefthook-local.yml
config to take precedence over everything else. Even more so because of this comment: https://github.com/evilmartians/lefthook/blob/8959135e2f4da83b44edde075bd16d581492f0c2/internal/config/load.go#L97-L102Actual results
Options from the
extends
files will take precedence.Possible Solution
A potential fix could involve modifying the merge process to ensure that
extends
files do not get reapplied. This could be achieved by filtering out previously appliedextends
files during the merging process to prevent them from being in the list whenextends
is called after merging each config source: