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?

Could we reject at the same time we select? e.g. f ~! /~$/ && File.executable?(f)

Sorry this should be !~. ;)

well. yes. but that decreases code readability

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

Mentioned in commit glensc/gitlab-ce@c549dda9

Mentioned in merge request gitlab-ce!6721 (merged)

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!

We already have a 4.0.0! If this doesn't break any existing workflows, then I think 4.1.0 makes sense. The versioning for this repo could pretty much be an incrementing integer anyway, given how we use it 😕

this came from master branch. i think rebase again with master will solve this bogus commit.

👍

Please credit yourself here! I also think it would be nice to credit @dirker, given that you both worked on the same feature independently, even if none of the code from the other MR ends up in here.

will do, @dirker's code is underneath my commits :)

added, wasn't sure how. as this is first entry that gets credited in changelog file

Thanks! There are credited entries further down (https://gitlab.com/gitlab-org/gitlab-shell/blob/master/CHANGELOG#L63), but that's exactly right 👍

On 17.11.2016 12:57, Sean McGivern wrote:

looks like email replies are broken,

but the email i sent said:

never link to branch, use commit or tag to refer to files like this :) your line offset is already off!

Touché, fair point! 😀

Isn't this required in gitlab_post_receive? Why do we need it here?

it came with !89 (closed), will check their discussion if any

Thanks 👍

it was added to have ROOT_PATH symbol available; this is no longer needed. removed in 2a040173

Dir["#{path}/[^~]*"] might work?

the intention of select block was to extend it in the future for more exclusions (.bak comes to mind). i would leave it as is.

OK, can we use String#start_with? in that case please?

changed in f253a00a.

Let's just make match_hook_files return an empty array if the path doesn't exist, and then we can remove this duplication.

changed in 5d65423e.

I don't think we need this method, maybe just a comment? 🙂

6f3305a9.

ok -> successful, here and below

c63e9ca2.

This keeps the filename, but I'd like to test the sorting. Can we add a test for that? Like you have in the MR description:

• 01-hook1.sh
• 02-hook2.pl
• 03-hook3.py

how do you suggest to test sorting?

Can we symlink the hooks to names like that, and test the same way as we test the ordering otherwise?

i haven't done that part of testing because don't know how to test the ordering. asked for help too. i mean how do i ensure the hooks are invoked in specific order. does the order of expect() methods require methods fired in that order?

i had some ideas to make each hook append to a file, and then test the result of the appended file.

also, here's one comment i wished to get answer: !93 (comment 18695337)

Aha, I see - I thought you were asking in general.

Well, we have two cases:

1. Hook earlier fails, so later hook shouldn't run - we can test this like we do now.
2. All hooks succeed. For this, we can add ordered to the message expecations.

Can we move this methods to above the specs, please? I tend to read specs top-to-bottom.

40f97c7f

btw, i personally prefer not to see those helpers at first sight. like private methods are defined bottom of the class.

Can we DRY up these a little bit, please? Maybe use a helper method?

i've just updated paths from !89 (closed), tried to change less as possible

i don't know what you have in mind here. too much abstraction breaks readability as well.

Actually, you're right. Now the rest of the specs are moved around, I think this is fine 👍

We also need to do this before, in case the spec run was aborted for whatever reason. Let's move it to a helper and call it in both 👍

i'm new to ruby and rspec, what is helper? :)

you mean add some class to spec/spec_helper.rb ?

Just a helper method, like you have further down with def. Nothing more complicated than that 😃

9db5caa8

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

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)

This is failing on CI, can you take a a look? Example failure:

  1) GitlabCustomHook with gl_id_test_hook pre_receive hook passes GL_ID variable to hook
     Failure/Error: FileUtils.symlink(File.join(tmp_root_path, 'hooks'), File.join(tmp_repo_path, 'hooks'))
     Errno::ENOENT:
       No such file or directory @ rb_file_s_symlink - (/builds/glensc/gitlab-shell/tmp/hooks, /builds/glensc/gitlab-shell/tmp/repo.git/hooks)
     # ./spec/gitlab_custom_hook_spec.rb:73:in `block (2 levels) in <top (required)>'

the GL_ID test needs to be rewritten due the problem i reported it's passing while it should not. there were instructions already given above somewhere.

i've fixed GL_ID test locally (866c8ee7), don't know why it fails in CI.

however, how do you suggest to test variations of before setup, should test local hook, global hook separately, not in same iteration:

https://gitlab.com/gitlab-org/gitlab-shell/blob/866c8ee717fe364214d4e6c054d0be7332dd4642/spec/gitlab_custom_hook_spec.rb#L86-89

as for the CI failure, what is supposed to setup the tmp/repo.git? as given the error, i suspect tmp/repo.git path is missing.

  1) GitlabCustomHook with gl_id_test_hook pre_receive hook passes GL_ID variable to hook
     Failure/Error: FileUtils.symlink(File.join(tmp_root_path, 'hooks'), File.join(tmp_repo_path, 'hooks'))
     Errno::ENOENT:
       No such file or directory @ sys_fail2 - (/builds/glensc/gitlab-shell/tmp/hooks, /builds/glensc/gitlab-shell/tmp/repo.git/hooks)
     # ./spec/gitlab_custom_hook_spec.rb:73:in `block (2 levels) in <top (required)>'

if i grep for repo.git for gitlab-shell codebase, i find nothing.

EDIT: probably fixed in 30bda7eb

however, how do you suggest to test variations of before setup, should test local hook, global hook separately, not in same iteration:

We can create separate contexts for 'with local hooks', 'with global hooks', and just do the different befores in there.

@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

Mentioned in commit glensc/gitlab-ce@896eae05

Mentioned in commit glensc/gitlab-ce@2091b12a

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)

mentioned in commit glensc/gitlab-ce@e7353406

mentioned in commit glensc/gitlab-ce@bc380e48

mentioned in issue gitlab-ce#14243 (closed)