Add global custom hooks

This set of commits implements a first proposal for the feature requested in #32 (closed), namely to have a set of global custom git hooks.

It extens the current GitlabCustomHook class to also look for hooks in "gitlab-shell/custom_hooks", and execute them if all other hooks (normal hook, repository local custom hook) were successful.

mentioned in issue #32 (closed)

It would be good to make the path to the global set of custom hooks configurable, especially for omnibus installations. I'd be happy if someone could point me towards the right place to add this to (add to GitlabConfig?).

@jacobvosmaer-gitlab Being a gitlab-shell expert, would you mind taking a look at this?

Please note that if we do accept this, we will need to double-check if there are any changes to GitLab wrt web commits (I think so).

@dblessing I think web commits are not affected. They just call out to the hooks in the current repository, which symlink to the code here in gitlab-shell.

This change looks reasonable. (I have a comment or two about the Ruby.) I am not sure if we should make the global custom hooks directory configurable in gitlab-shell; this can be done in omnibus-gitlab too by putting a symlink at /opt/gitlab/embedded/service/gitlab-shell/custom_hooks.

We should have some tests for CustomHook if we do this.

Added 1 commit:

• f802440c - custom_hook: add support for global custom hooks

Added 1 commit:

• 79d37ae7 - custom_hook: add support for global custom hooks

@jacobvosmaer-gitlab thanks for reviewing. Incorporated your suggestions, this is a much nicer solution of course.

I'd be happy to add tests, but since I'm no ruby developer and therefore also not familiar with rspec, how would I go about adding stubs or fixtures for the hooks to call in my tests?

I would create fixtures in tmp/ with FileUtils and stub out ROOT_PATH, system and call_receive_hook.

@DouweM do you agree this needs tests? I feel a little bad putting that on somebody who has never written Rspec before.

Personally I also think this needs some testing: the order of evaluation and proper handling of return results. Especially if hooks are used to implement further permission checks. I'll see what I can do.

Added 2 commits:

• 5a6455ce - custom_hook: add support for global custom hooks
• 81fe5255 - Add specs for GitlabCustomHook

Gave adding specs for GitlabCustomHook a shot, please review. Thanks.

Looks great @dirker ! I am going to leave some comments.

Added 2 commits:

• b3cf2831 - custom_hook: add support for global custom hooks
• 01671dec - Add specs for GitlabCustomHook

Added 1 commit:

• ccc160d5 - Add specs for GitlabCustomHook

Incorporated your comments @jacobvosmaer-gitlab.

@dirker I found a few more 'cosmetic' things. Apart from those I am ready to pass this to the next reviewer.

Added 1 commit:

• d8d3da2f - Add specs for GitlabCustomHook

Reworked again, thanks for the comments @jacobvosmaer-gitlab. I left out the ordering on the "ok repo, failing global", although in that case it is three lists of length two. Maybe it should be in, however the order of execution also gets tested during "failing repo, ok global" case.

Hope that's ok.

Resolved all discussions

@DouweM would you be willing to do the final review?

Reassigned to @DouweM

Added 30 commits:

• d8d3da2f...590d0d9c - 26 commits from branch gitlab-org:master
• 61c98beb - custom_hook: only execute hook if file is executable
• c06e1a44 - custom_hook: refactor to pull repo_path into class
• 88d5a0c4 - custom_hook: add support for global custom hooks
• 698de16f - spec: add tests for global custom hooks

Hi all, I have rebased my branch against the current master in order to resolve the merge conflict introduced by adding back exporting GL_ID to all hooks. Hope it's still good to go.

Mentioned in merge request !93 (closed)

@DouweM could you review this?

@dirker how does what this MR does compare with !93 (closed)?

Mentioned in issue #32 (closed)

@jacobvosmaer-gitlab I think !93 (closed) is doing the same thing, plus more.

This merge request only extends the custom_hooks by one more location (gitlab-shell/custom_hooks).

!93 (closed) is extending the hook handling itself by supporting *.d directories (i.e. multiple hooks per type [pre-receive, update, post-receive]). !93 (closed) is also adding gitlab-shell/hooks/*.d, essentially implementing global custom_hooks.

Ultimately, I think having *.d directories is a nice idea. The question is if this must be supported by gitlab-shell directly, or whether it is enough to implement that functionality out of gitlab-shell in the custom_hooks themselves, yielding a (slightly) simpler implementation. On the other hand the functionality of calling multiple hooks along with the required data is there in gitlab-shell anyway, so why duplicate it inside of the custom_hooks.

Personally, I do not have a real preference, as long as any of the two merge requests finally gets merged in the near future. Been quite a while now for such a simple change.

Thanks @dirker . I think I prefer leaving the complexity of multiplexing hooks out of gitlab-shell.

@DouweM I propose we go with this MR instead of !93 (closed). Do you agree?

Being the !93 (closed) author, i think that multiplexing should be part of gitlab-shell, because all the infrastructure is already there and having the chance doing it rightfully. If everybody writes their own multiplexer, they could implement whatever detail wrong (not handling sigpipe or losing stdin from previous script, ...).

but having new path from !89 (closed) and new paths from !93 (closed) together seems like too many paths, so i'd see this being merged to temp branch and then !93 (closed) setting their paths. i don't know how that can be done technically. suggestions?

The change looks symmetric this way: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6721/diffs

Closing as per https://gitlab.com/gitlab-org/gitlab-shell/issues/32#note_18181189 - let's go with !93 (closed) for now.

Status changed to closed

Mentioned in merge request !111 (merged)