Custom hooks #190

dblessing · 2014-10-30T03:41:18Z

This custom hook implementation support pre-receive, post-receive and update hooks. If a gitlab administrator wants to add a custom hook they need to follow these steps:

Create a custom_hooks directory in the repository directory adjacent to the normal hooks directory. For example, if the repo path is /home/git/repositories/my_group/my_project.git, the custom hook path will be /home/git/repositories/my_group/my_project.git/custom_hooks
Create/place a hook of the desired type in this custom_hooks directory.
Make the hook file executable and owned by the git user.
Profit. The new custom hook should be picked up by gitlab-shell on the next push. If there are errors in the custom hook the push may fail, unless it's post-receive. If the hook is not owned by git or is not executable the custom hook will silently fail.

What's missing?

Other hooks types - pre-commit/etc

dblessing · 2014-10-30T03:46:30Z

lib/gitlab_custom_hook.rb

+  def hook_files(type)
+    # Get all files for this repo and hook type.
+    # i.e. ${custom_hooks_dir}/${namespace}/${repo_name}/(pre-receive|update|etc)
+  end


Actually, this would not be multiple files. Only one hook per type per repo.

dzaporozhets · 2014-10-31T09:36:29Z

The idea is ok. Ping me when ready for review

jkbzh · 2014-10-31T10:18:37Z

hooks/pre-receive

-else
-  exit 1
-end
+exit 1 unless GitlabCustomHook.new.pre_receive(repo_path, key_id, refs).exec


hi @dblessing. Good job. Where would the custom hooks be located?
Some comments on the pre-receive hook changes:

You're calling the custom hook before the gitlab acccess control check. I think this is akin to making a pre-authorization hook and the hook name should reflect that.

You should also have a call to the custom hook after the gitlab hook access control check. You can call this one the pre-receive one as it will then be equivalent (in my eyes) to where a standard git hook would be called. For example, if accessing that repo thru ssh.
If you only intend to have one such call to a custom hook, I'd give a preference to have this be for a custom pre-receive hook and that it be called after the gitlab access control check.

The gitlab pre-receive hooks sets up an environment variable called 'GL_ID' . Further on, the exec method in the gitLabCustomHook exec method unsets this variable if the access control check fails. A comment there says "reset GL_ID env since we stop git push here" (gitlab_access.rb). I didn't look into detail into the code to see why this environment variable has to be setup. However, as it's set up in one place and unset elsewhere, this seems to point out that you should do the same before the call to exit 1. You could do this in the GitlabCustomHook function and document that the custom hook must return true or false rather than exit.
However, a best practice would be to set and unset those variables in the same place. That's what I would do myself and instead of having multiple exit 1, just have a single exit 0 and and an exit 1 and unset the env. variable before the call to the exit 1.

houndci-bot · 2014-11-04T11:52:27Z

lib/gitlab_custom_hook.rb

+  end
+
+  def hook_file(hook_type, repo_path)
+    hook_path = repo_path.gsub(/hooks$/, 'custom_hooks')


Useless assignment to variable - hook_path.

dblessing · 2014-11-04T11:57:13Z

@jkbzh I have a possible implementation (see commits). I'm still testing, but if you're interested I would appreciate some further testing by you and others in development/test environments. I'm not yet certain args are being passed correctly.

houndci-bot · 2014-11-04T13:55:48Z

lib/gitlab_custom_hook.rb

+  def pre_receive(old_value, new_value, ref_name, repo_path)
+    hook = hook_file('pre-receive', repo_path)
+    return 0 if hook.nil?
+    return system(*hook, old_value, new_value, ref_name) ? 0 : 1


Redundant return detected.

houndci-bot · 2014-11-04T14:32:01Z

lib/gitlab_post_receive.rb

@@ -23,9 +23,12 @@ def exec
  def update_redis
    queue = "#{config.redis_namespace}:queue:post_receive"
    msg = JSON.dump({'class' => 'PostReceive', 'args' => [@repo_path, @actor, @changes]})
-    unless system(*config.redis_command, 'rpush', queue, msg, err: '/dev/null', out: '/dev/null')
+    if system(*config.redis_command, 'rpush', queue, msg,
+               err: '/dev/null', out: '/dev/null')


Align the parameters of a method call if they span more than one line.

dblessing · 2014-11-04T14:34:22Z

@randx I think this is ready for your review.

dblessing · 2014-11-04T14:53:37Z

There's not a really great way to document this in gitlab-shell. Should we put something in GitLab docs instead?

dblessing · 2014-11-05T12:21:54Z

@randx Fixed return values

jkbzh · 2014-11-05T12:53:45Z

@dblessing forked and look at it right now. Could you tell me where are the arguments you're not sure you're passing to and in which files? If we could irc that would save time.

dblessing · 2014-11-05T13:29:13Z

@jkbzh I'll be on #gitlab IRC in about 45 min. (dblessing). I'm interested in whether I have the right args in the right order for the diff hook types. For example, system(*hook, ref_name, old_value, new_value) for update hooks. As best I can tell based on git hook docs I do have them right but I'd love some validation.

jkbzh · 2014-11-05T15:17:26Z

@dblessing comparing against a stand-alone git setting, the hooks expect to be feed the changes line thru stdin, not as cli arguments. I'll shortly send a PR with a modified gitlab_custom_hook.rb that shows how to do it.

houndci-bot · 2014-11-05T21:00:12Z

lib/gitlab_custom_hook.rb

+
+  def receive(type, refs, repo_path)
+    unless type == 'pre-receive' || type == 'post-receive'
+      puts "GitLab: An unexpected error occurred " \


Prefer single-quoted strings when you don't need string interpolation or special symbols.

dblessing · 2014-11-05T21:01:31Z

@jkbzh I updated this PR so the pre-receive now should receive all stdin. Post-receive already supported this. Update hook is called once per branch pushed so it does not need this type of support.

houndci-bot · 2014-11-06T14:36:09Z

lib/gitlab_custom_hook.rb

+    hook_file = "#{hook_path}/#{hook_type}"
+    hook_file if File.exist?(hook_file)
+  end
+


Extra empty line detected at body end.

dblessing · 2014-11-06T14:50:26Z

Ok, we're really ready for review now, @randx. Thanks to help from @jkbzh we have a nice working implementation.

dzaporozhets · 2014-11-06T16:19:33Z

lib/gitlab_custom_hook.rb

+  private
+
+  def call_receive_hook(hook, changes)
+    require 'open3'


I dont like require inside method

- Reset G_ID if the custom pre-receive hook fails - Use a pipe to feed stdin to the custom pre- and post-receive hooks, in the same way that the standalone git works

Custom hooks

dzaporozhets · 2014-11-07T13:19:33Z

Looks good! Thank you @dblessing and @jkbzh

dzaporozhets · 2014-11-07T13:20:38Z

One thing I forgot to ask is to update CHANGELOG. Can you please submit PR for this?

dblessing · 2014-11-07T14:18:51Z

@randx Which version should this be under? v2.1.0 is already released, correct? What will the next version be?

dzaporozhets · 2014-11-07T14:19:28Z

2.2.0

dosire · 2014-11-09T19:11:17Z

@dblessing Would you be so kind to add documentation for this in the doc directory? I think it can be linked from the administrator documentation http://doc.gitlab.com/ce/

dosire · 2014-11-09T19:11:49Z

@dblessing Just noticed you already made https://github.com/gitlabhq/gitlabhq/pull/8276/files Awesome!

coveralls · 2016-04-14T03:26:57Z

Coverage decreased (-0.07%) to 98.01% when pulling 72e21a4 on dblessing:feature/custom_hooks into cc193ea on gitlabhq:master.

coveralls · 2016-04-14T03:31:48Z

Coverage decreased (-1.3%) to 96.823% when pulling 72e21a4 on dblessing:feature/custom_hooks into cc193ea on gitlabhq:master.

dblessing reviewed Oct 30, 2014
View reviewed changes

jkbzh reviewed Oct 31, 2014
View reviewed changes

jkbzh mentioned this pull request Oct 31, 2014

Custom post-receive hooks can't be called #194

Closed

houndci-bot reviewed Nov 4, 2014
View reviewed changes

dblessing force-pushed the feature/custom_hooks branch 3 times, most recently from a96367f to a876393 Compare November 4, 2014 14:31

houndci-bot reviewed Nov 4, 2014
View reviewed changes

dblessing force-pushed the feature/custom_hooks branch from a876393 to 7f95e3f Compare November 4, 2014 14:33

dblessing changed the title ~~WIP - Custom hooks POC~~ Custom hooks Nov 4, 2014

dblessing force-pushed the feature/custom_hooks branch from 7f95e3f to d082978 Compare November 5, 2014 12:20

dblessing force-pushed the feature/custom_hooks branch from d082978 to ce73915 Compare November 5, 2014 21:00

houndci-bot reviewed Nov 5, 2014
View reviewed changes

dblessing force-pushed the feature/custom_hooks branch from ce73915 to ac6338c Compare November 5, 2014 21:00

Support for custom hooks

3ef7cdf

dblessing force-pushed the feature/custom_hooks branch from ac6338c to 3ef7cdf Compare November 5, 2014 21:16

houndci-bot reviewed Nov 6, 2014
View reviewed changes

dblessing force-pushed the feature/custom_hooks branch from 786c378 to 668e583 Compare November 6, 2014 14:48

dzaporozhets reviewed Nov 6, 2014
View reviewed changes

lib/gitlab_custom_hook.rb

private

def call_receive_hook(hook, changes)

require 'open3'

Copy link

Contributor

dzaporozhets Nov 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont like require inside method

Fix the calls to pre- and post-receive custom hooks

b6d84f6

- Reset G_ID if the custom pre-receive hook fails - Use a pipe to feed stdin to the custom pre- and post-receive hooks, in the same way that the standalone git works

dblessing force-pushed the feature/custom_hooks branch from 668e583 to b6d84f6 Compare November 6, 2014 19:26

dzaporozhets added a commit that referenced this pull request Nov 7, 2014

Merge pull request #190 from dblessing/feature/custom_hooks

ff9808c

Custom hooks

dzaporozhets merged commit ff9808c into gitlabhq:master Nov 7, 2014

This was referenced Nov 7, 2014

Add custom hook support to CHANGELOG #196

Merged

Custom git hook documentation gitlabhq/gitlabhq#8276

Merged

Kami-no mentioned this pull request Nov 14, 2014

gitlab-shell-2.2.0.ebuild required fritteli/gentoo-overlay#2

Closed

dzaporozhets mentioned this pull request Nov 17, 2014

Restore hooks PATH before calling ruby. #186

Merged

Custom hooks #190

Custom hooks #190

Uh oh!

Conversation

dblessing commented Oct 30, 2014

What's missing?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzaporozhets commented Oct 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblessing commented Nov 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblessing commented Nov 4, 2014

Uh oh!

dblessing commented Nov 4, 2014

Uh oh!

dblessing commented Nov 5, 2014

Uh oh!

jkbzh commented Nov 5, 2014

Uh oh!

dblessing commented Nov 5, 2014

Uh oh!

jkbzh commented Nov 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblessing commented Nov 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblessing commented Nov 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzaporozhets commented Nov 7, 2014

Uh oh!

dzaporozhets commented Nov 7, 2014

Uh oh!

dblessing commented Nov 7, 2014

Uh oh!

dzaporozhets commented Nov 7, 2014

Uh oh!

dosire commented Nov 9, 2014

Uh oh!

dosire commented Nov 9, 2014

Uh oh!

coveralls commented Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commented Apr 14, 2016 •

edited

Loading

coveralls commented Apr 14, 2016 •

edited

Loading