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:

1. process per project hooks <repository>.git/custom_hooks/<hook_name>.d/* as <repository>.git/custom_hooks is local dir for <repository>.git
2. process global hooks from <repository>.git/hooks/<hook_name>.d/* because <repository>.git/hooks is symlink to gitlab-shell/hooks

the hooks matched by shell glob must be:

1. executable (+x bit set)
2. not matching editor backup files (*~)

the overview of the whole process:

• <repository>.git/hooks/ - symlink to gitlab-shell/hooks global dir
• <repository>.git/hooks/<hook_name> - executed by git itself, this is gitlab-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:

1. 01-hook1.sh
2. 02-hook2.pl
3. 03-hook3.py

Docs MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6721

Changed title: WIP: Chain custom hooks in <hook_name>.d dir → WIP: Chain custom hooks in <hook_name>.d dirs

Added 1 commit:

• a163fd2a - redo hooks lookup to use <hook>.d/* from repository hooks dir

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)

Added 1 commit:

• b5ba986c - redo hooks lookup to use <hook>.d/* from repository hooks dir

Added 1 commit:

• 1f835a35 - remove excess doc

Added 1 commit:

• 671600ae - redo hooks lookup to use <hook>.d/* from repository hooks dir

Unmarked this merge request as a Work In Progress

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

Added 1 commit:

• 4baa5325 - fix indent

Added 1 commit:

• 25addbb3 - redo hooks lookup to use <hook>.d/* from repository hooks dir

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

Mentioned in merge request !95 (merged)

@rymai, would you be able to take a look at this please?

@glensc Thanks for your contribution!

@dblessing Would you mind just giving your thoughts on this since you've done the original custom hooks implementation?

Added 1 commit:

• caffdeb3 - filter names and executable in single iteration

Resolved all discussions

@glensc Thanks! LGTM, let's wait for @dblessing's feedback! :)

@glensc I'm unclear on one piece.

process global hooks from <repository>.git/hooks/<hook_name>.d/* because <repository>.git/hooks is symlink to gitlab-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 at gitlab-shell/hooks/<hook_name>.d/*?

If that's the case, then this is great.

As a follow up, we need to update docs in gitlab-ce/docs. @glensc would you like to do that?

yes, <repository>.git/hooks is symlink so logically taking <repository>.git/hooks/<hook_name>.d/* are global hooks because they reside in gitlab-shell/hooks/<hook_name>.d/*

great to see this is getting accepted!

should i squash commits here and add changelog entry?

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.

Thanks @glensc! Could you add tests, though?

Adding tests takes some time, rspec (and Ruby) kind a new to me

@glensc No problem, don't hesitate to ask if you need any help!

I have not started yet, so i do not have questions, but if you have good examples, share them :)

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

@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?

@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>?

Mentioned in merge request !89 (closed)

Mentioned in issue #32 (closed)

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

Compare with previous version

Added 3 commits:

• cb6aa61b...0f8a3cbb - 2 commits from branch gitlab-org:master
• 28fc0f5c - Chain custom hooks

Compare with previous version

Added 2 commits:

• ed59fbc3 - 1 commit from branch gitlab-org:master
• 8845fc26 - Chain custom hooks

Compare with previous version

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

Compare with previous version

Added 1 commit:

• 995b8f6e - updated tests to match current code

Compare with previous version

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.

@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 failure

This is because we use allow, which won't fail the spec if the method isn't called. Changing that to expect 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 see

I will add comments to the MR now!

@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!

Reassigned to @glensc

Milestone changed to %8.15

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

Compare with previous version

i've reordered the projects vs global hooks order as it was in dirk's code: f4d0c492d38e40b54d7e9b0b683b2785c9af9797

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

Added 1 commit:

• 2a040173 - remove no longer needed gitlab_init

Compare with previous version

Added 2 commits:

• f253a00a - use String.end_with? instead of regexp
• 5d65423e - avoid Dir.exists? duplication by moving the check to match_hook_files

Compare with previous version

Added 2 commits:

• 6f3305a9 - remove unused create_global_hooks
• c63e9ca2 - improve wording by using successful instead of ok

Compare with previous version

Added 1 commit:

• f822ccca - changelog entry

Compare with previous version

Hi @glensc, just wanted to say thanks for following through so this feature can finally get merged . @smcgivern thanks for reviewing!

@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.

Added 1 commit:

• 40f97c7f - move helpers to top

Compare with previous version

Added 1 commit:

• 9db5caa8 - cleanup dirs in before to fixup aborted tests

Compare with previous version

also who should mark "discussion resolved"?

and perhaps merge without further tests, it seems accepted here: !106 (merged) which actually broke something: #70 (closed)

@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

Compare with previous version

Added 1 commit:

• 866c8ee7 - fix gl_id_test_hook

Compare with previous version

Added 1 commit:

• c2f4a9da - spec/custom_hooks: cleanup helpers not to repeat repo path parameter

Compare with previous version

Added 1 commit:

• 30bda7eb - spec/custom_hook: ensure "before" does complete cleanup

Compare with previous version

@glensc thanks! I think I just have one comment left.

Added 3 commits:

• 345c7bc7 - test gl_id_test_hook as global and as repo hook separately
• 782356bf - DRY: add helpers for expect hook calls
• bda3016e - test expected hook order

Compare with previous version

Resolved all discussions

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

Added 1 commit:

• b28718cf - changelog spelling

Compare with previous version

Reassigned to @smcgivern

@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

@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.

@glensc I did rebase! I just can't push to your fork

the new MR is at !111 (merged)