Add support for global custom hooks and chained hook directories
This is continuation of PR#245 from GitHub and !89 (closed) addressing #32 (closed)
With changes of mine:
- process per project hooks
<repository>.git/custom_hooks/<hook_name>.d/*
as<repository>.git/custom_hooks
is local dir for<repository>.git
- process global hooks from
<repository>.git/hooks/<hook_name>.d/*
because<repository>.git/hooks
is symlink togitlab-shell/hooks
the hooks matched by shell glob must be:
- executable (
+x
bit set) - not matching editor backup files (
*~
)
the overview of the whole process:
-
<repository>.git/hooks/
- symlink togitlab-shell/hooks
global dir -
<repository>.git/hooks/<hook_name>
- executed bygit
itself, this isgitlab-shell/hooks/<hook_name>
-
<repository>.git/custom_hooks/<hook_name>
- per project hook (this is already existing behavior) -
<repository>.git/custom_hooks/<hook_name>.d/*
- per project hooks -
<repository>.git/hooks/<hook_name>.d/*
- global hooks: all executable files (minus editor backup files)
the files matched by glob are also sorted so if order of hooks is important, one can name their hooks as:
- 01-hook1.sh
- 02-hook2.pl
- 03-hook3.py
Docs MR: gitlab-ce!6721 (merged)
-
Changed title: WIP: Chain custom hooks in <hook_name>.d dir → WIP: Chain custom hooks in <hook_name>.d dirs
Toggle commit list -
1
some unresolved topics:
- should all hooks be ran even if any of the hooks fail? current implementation skips rest of the hooks if any of the hooks fails
- what should be the order of hooks be ran (global hooks vs per-project hooks)
-
Unmarked this merge request as a Work In Progress
Toggle commit list -
1
i think it's ok if any hook exits with failure, that cascades that whole push would be rejected
as for sort order, for me it is not important what it is, so the current order would be ok
-
1
as for documentation, found two places that need documentation update:
- https://gitlab.com/help/administration/custom_hooks.md
- https://docs.gitlab.com/ce/administration/custom_hooks.html
both source seems to be in
gitlab-ce
repository: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/administration/custom_hooks.md -
Added 5 commits:
-
25addbb3...0b4fd0af - 3 commits from branch
gitlab-org:master
- bd5e0ea3 - Chain custom hooks
- 2319eb30 - redo hooks lookup to use <hook>.d/* from repository hooks dir
Toggle commit list -
25addbb3...0b4fd0af - 3 commits from branch
-
-
Master
@rymai, would you be able to take a look at this please?
-
-
Master
@glensc Thanks for your contribution!
@dblessing Would you mind just giving your thoughts on this since you've done the original custom hooks implementation?
-
Resolved all discussions
Toggle commit list -
Master
@glensc Thanks! LGTM, let's wait for @dblessing's feedback! :)
-
Master
@glensc I'm unclear on one piece.
process global hooks from
<repository>.git/hooks/<hook_name>.d/*
because<repository>.git/hooks
is symlink togitlab-shell/hooks
I was unclear because you only mention
<repository>.git/hooks/<hook_name>.d/*
, but in reality this is a symlink and the actual files are atgitlab-shell/hooks/<hook_name>.d/*
?If that's the case, then this is great.
👍 -
Master
As a follow up, we need to update docs in
gitlab-ce/docs
. @glensc would you like to do that? -
1
yes,
<repository>.git/hooks
is symlink so logically taking<repository>.git/hooks/<hook_name>.d/*
are global hooks because they reside ingitlab-shell/hooks/<hook_name>.d/*
-
1
great to see this is getting accepted!
should i squash commits here and add changelog entry?
-
-
-
1
Doc update MR created (WIP), what to document there? copy whole MR description there? also I suppose it should say something about versioning? Actually please respond in the doc MR not here.
-
1
Adding tests takes some time, rspec (and Ruby) kind a new to me
-
1
I have not started yet, so i do not have questions, but if you have good examples, share them :)
-
1
found similar issue: #32 (closed) and it's MR: !89 (closed)
that seems to intersect with problems being solved here. why this isn't pointed out? nobody knows what's the issue/merge-request backlog?
@dblessing commented on both MR's, so at least he should know.
from what i see, i'd like to use tests base from the other MR and then perhaps the other MR becames pointless because this MR adds
*.d
directory support.puzzled
-
Master
@glensc Sorry about that! Ideally, each merge request should address a specific issue, so that we avoid having two contributors working on the same thing!
That being said, there's definitely an intersection between those two MRs, but I think both are useful and address a particular problem:
- !89 (closed) allows to define global hooks
- !93 (closed) allows to define multiple hooks of the same type in order to chain them
@glensc It seems that !89 (closed) is older and mostly ready, so maybe you could rebase upon
master
once it's merged? @dblessing Does it make sense? -
1
@rymai but !89 (closed) defines global hooks dir that is somewhat superfluous if !93 (closed) gets merged.
i think is sufficient to have
gitlab-shell/hooks/<hook_name>.d
for having global hooks, can this PR revert that part to have not so many directories for same purpose?or it's rather good and symmetric to have
gitlab-shell/custom_hooks/<hook_name>
as there is<repository>/custom_hooks/<hook_name>
? -
-
-
1
rebased against current master. altho i'd really prefer rebase against !89 (closed)
-
Added 21 commits:
-
caffdeb3...eae98b67 - 20 commits from branch
gitlab-org:master
- cb6aa61b - Chain custom hooks
Toggle commit list -
caffdeb3...eae98b67 - 20 commits from branch
-
Added 3 commits:
-
cb6aa61b...0f8a3cbb - 2 commits from branch
gitlab-org:master
- 28fc0f5c - Chain custom hooks
Toggle commit list -
cb6aa61b...0f8a3cbb - 2 commits from branch
-
Added 2 commits:
Toggle commit list -
1
ok, as i see no movement in either direction, and nobody answering direction either,
i've rebased my MR !89 (closed)
-
Added 7 commits:
- 3288099a - custom_hook: only execute hook if file is executable
- 6fc6cef2 - custom_hook: refactor to pull repo_path into class
- da7423a0 - custom_hook: add support for global custom hooks
- 98410d2d - spec: add tests for global custom hooks
- 9b46246e - Use full repository path for API calls instead of extracting name
- a4891275 - Release 4.0.0
- 82e3d76b - Chain custom hooks
Toggle commit list -
Toggle commit list
-
1
i've updated rspec tests to match this MR changes, and noticed that hooks order differs with the one from !89 (closed). i've updated tests to match that. should the order be revised?
also noticed that
GL_ID
test is broken, it asserts methods, those are never called but rspec does not result failure. i'm new to rspec, so any hints how to fix that? (hook_file
method no longer exists so it is not called)also the variants of
.d
are not fully tested (and combinations), suggest which tests you want to see. i don't want to put too much time into this "just in case" if the MR gets ditched. -
Master
@glensc I'm really sorry about the time this has taken, and thanks so much for your patience with this. We're going to go with this MR: I think the approach that you've outlined in the description is really clear, consistent, and flexible
👍 should the order be revised?
Can you explain the difference? As I understand it, we will execute the new global hooks before the existing project-specific hooks. And the other MR would execute the project's hooks, then the global hooks?
also noticed that
GL_ID
test is broken, it asserts methods, those are never called but rspec does not result failureThis is because we use
allow
, which won't fail the spec if the method isn't called. Changing that toexpect
will fail if it isn't called, and I think it makes sense to do that.also the variants of
.d
are not fully tested (and combinations), suggest which tests you want to seeI will add comments to the MR now!
-
-
-
-
-
-
-
-
-
-
-
-
Master
@glensc can you take a look at the comments above please? And as master was very recently updated, it would be good to update this MR to match as there are currently some unrelated changes showing in the diffs tab. Let me know if you need any more help, and thanks again!
🌠 -
-
-
Added 9 commits:
-
995b8f6e...ed59fbc3 - 3 commits from branch
gitlab-org:master
- 5a5e5c2c - custom_hook: only execute hook if file is executable
- d1c30a96 - custom_hook: refactor to pull repo_path into class
- f4d0c492 - custom_hook: add support for global custom hooks
- 10366ee2 - spec: add tests for global custom hooks
- f89ccecf - custom_hook: chain custom hooks
- 0fa85f91 - spec: updated tests to match current code
Toggle commit list -
995b8f6e...ed59fbc3 - 3 commits from branch
-
1
i've reordered the projects vs global hooks order as it was in dirk's code: f4d0c492
The repository local custom hook is executed first (if available). If successful, execution continues with the global custom hook (if available). This way, local custom hooks get priority over global custom hooks.
i don't have strong opinion on the order, and that commit was only one that had an opinion with a justification.
i'll have look at the other MR comments soon
-
Toggle commit list
-
-
Hi @glensc, just wanted to say thanks for following through so this feature can finally get merged
👍 . @smcgivern thanks for reviewing! -
1
@smcgivern looks like the 4.0.1 changes require rebase with master again. any suggestion how the commits be handled? as if you look at the MR commits, this MR is based on dirk's changes, and the changes on 4.0.1 will need to modify dirk commits. the second issue: i've done commits with the review. will the MR be merged as is, or they should be squashed somehow? could at least changes made after last review be reviewed. i've still not gone through the testing (spec) changes.
-
-
-
1
also who should mark "discussion resolved"?
-
1
and perhaps merge without further tests, it seems accepted here: !106 (merged) which actually broke something: #70 (closed)
-
-
Master
@glensc thanks! I've resolved all but one of the comments here:
looks like the 4.0.1 changes require rebase with master again. any suggestion how the commits be handled? as if you look at the MR commits, this MR is based on dirk's changes, and the changes on 4.0.1 will need to modify dirk commits.
You should be able to fix the conflicts while rebasing, but if you're finding it difficult, let me know and I can help.
the second issue: i've done commits with the review. will the MR be merged as is, or they should be squashed somehow?
I'd
❤ it if you could squash them into nice atomic commits when rebasing, but it's not required.also who should mark "discussion resolved"?
Either of us! In the case where you link to a commit SHA resolving the comment, that's a perfect time to resolve the discussion too.
and perhaps merge without further tests, it seems accepted here: !106 (merged) which actually broke something: #70 (closed)
Right, but we shouldn't have done that
😉 I still need to test this manually as well, once it's green on CI and ready to merge! -
Added 21 commits:
-
9db5caa8...d7428a5a - 7 commits from branch
gitlab-org:master
- cae5590d - custom_hook: only execute hook if file is executable
- 891760c6 - custom_hook: refactor to pull repo_path into class
- 29ddbf7b - custom_hook: add support for global custom hooks
- 78a837ec - spec: add tests for global custom hooks
- 28e12d3b - custom_hook: chain custom hooks
- ca3f782a - spec: updated tests to match current code
- a4764440 - remove no longer needed gitlab_init
- ec31c4db - use String.end_with? instead of regexp
- ed2eeb3c - avoid Dir.exists? duplication by moving the check to match_hook_files
- b43619bb - remove unused create_global_hooks
- 8aaf14b0 - improve wording by using successful instead of ok
- 60548f23 - changelog entry
- 4d67a22c - move helpers to top
- 47b3a192 - cleanup dirs in before to fixup aborted tests
Toggle commit list -
9db5caa8...d7428a5a - 7 commits from branch
-
-
Added 1 commit:
- c2f4a9da - spec/custom_hooks: cleanup helpers not to repeat repo path parameter
Toggle commit list -
Added 1 commit:
- 30bda7eb - spec/custom_hook: ensure "before" does complete cleanup
Toggle commit list -
Master
@glensc thanks! I think I just have one comment left.
-
Resolved all discussions
Toggle commit list -
1
finally. lets merge this!
i'm really tired of this MR, if someone wants to nitpick on some of the tests, he/she should do it in separate MR theirselves :)
-
Changed title: chain custom hooks in <hook_name>.d dirs → Add support for global custom hooks and chained hook directories
Toggle commit list -
-
-
-
-
Master
@glensc thank you for your work on this! There is a conflict, so I'll create a new MR to fix that and close this one!
-
Status changed to closed
Toggle commit list -
1
@smcgivern why not just rebase? what kind of conflict? also would be nice to link to the new MR from here so i could follow it.
-
Master
@glensc I did rebase! I just can't push to your fork
🙂 -
1
the new MR is at !111 (merged)
-
-
-