Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • This project
    • Loading...
  • Sign in / Register
gitlab-shell
gitlab-shell
  • Overview
    • Overview
    • Details
    • Activity
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
    • Locked Files
  • Issues 32
    • Issues 32
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
  • Merge Requests 18
    • Merge Requests 18
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Charts
  • Wiki
    • Wiki
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • GitLab.org
  • gitlab-shellgitlab-shell
  • Merge Requests
  • !93

Closed
Opened Sep 23, 2016 by Elan Ruusamäe@glensc 1
  • Report abuse
Report abuse

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: gitlab-ce!6721 (merged)

×

Check out, review, and merge locally

Step 1. Fetch and check out the branch for this merge request

git fetch https://gitlab.com/glensc/gitlab-shell.git pr-245
git checkout -b glensc/gitlab-shell-pr-245 FETCH_HEAD

Step 2. Review the changes locally

Step 3. Merge the branch and fix any conflicts that come up

git checkout master
git merge --no-ff glensc/gitlab-shell-pr-245

Step 4. Push the result of the merge to GitLab

git push origin master

Note that pushing to GitLab requires write access to this repository.

Tip: You can also checkout merge requests locally by following these guidelines.

  • Discussion 84
  • Commits 21
  • Pipelines 27
  • Changes 10
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
  • Elan Ruusamäe @glensc

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

    Sep 23, 2016

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

    Changed title: **WIP: Chain custom hooks in &lt;hook_name&gt;.d dir** → **WIP: Chain custom hooks in &lt;hook_name&gt;.d dir{+s+}**
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Added 1 commit:

    • a163fd2a - redo hooks lookup to use <hook>.d/* from repository hooks dir
    Added 1 commit: * a163fd2a - redo hooks lookup to use &lt;hook&gt;.d&#x2F;* from repository hooks dir
    Toggle commit list
  • Elan Ruusamäe @glensc commented Sep 23, 2016
    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)
    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)
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Added 1 commit:

    • b5ba986c - redo hooks lookup to use <hook>.d/* from repository hooks dir
    Added 1 commit: * b5ba986c - redo hooks lookup to use &lt;hook&gt;.d&#x2F;* from repository hooks dir
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 1f835a35 - remove excess doc
    Sep 23, 2016

    Added 1 commit:

    • 1f835a35 - remove excess doc
    Added 1 commit: * 1f835a35 - remove excess doc
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Added 1 commit:

    • 671600ae - redo hooks lookup to use <hook>.d/* from repository hooks dir
    Added 1 commit: * 671600ae - redo hooks lookup to use &lt;hook&gt;.d&#x2F;* from repository hooks dir
    Toggle commit list
  • Elan Ruusamäe @glensc

    Unmarked this merge request as a Work In Progress

    Sep 23, 2016

    Unmarked this merge request as a Work In Progress

    Unmarked this merge request as a Work In Progress
    Toggle commit list
  • Elan Ruusamäe @glensc commented Sep 23, 2016
    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

    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
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 4baa5325 - fix indent
    Sep 23, 2016

    Added 1 commit:

    • 4baa5325 - fix indent
    Added 1 commit: * 4baa5325 - fix indent
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Added 1 commit:

    • 25addbb3 - redo hooks lookup to use <hook>.d/* from repository hooks dir
    Added 1 commit: * 25addbb3 - redo hooks lookup to use &lt;hook&gt;.d&#x2F;* from repository hooks dir
    Toggle commit list
  • Elan Ruusamäe @glensc commented Sep 23, 2016
    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

    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
  • Elan Ruusamäe @glensc

    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
    Sep 27, 2016

    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
    Added 5 commits: * 25addbb3...0b4fd0af - 3 commits from branch `gitlab-org:master` * bd5e0ea3 - Chain custom hooks * 2319eb30 - redo hooks lookup to use &lt;hook&gt;.d&#x2F;* from repository hooks dir
    Toggle commit list
  • Elan Ruusamäe @glensc

    Mentioned in merge request !95 (merged)

    Oct 05, 2016

    Mentioned in merge request !95 (merged)

    Mentioned in merge request !95
    Toggle commit list
  • Sean McGivern @smcgivern commented Oct 05, 2016
    Master

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

    @rymai, would you be able to take a look at this please?
  • Rémy Coutable
    @rymai started a discussion on an old version of the diff Oct 06, 2016
    Resolved by Elan Ruusamäe Oct 06, 2016
    lib/gitlab_custom_hook.rb
    Unable to load the diff.
    • Rémy Coutable @rymai commented Oct 06, 2016
      Master

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

      Could we reject at the same time we select? e.g. `f ~! /~$/ && File.executable?(f)`
    • Rémy Coutable @rymai commented Oct 06, 2016
      Master

      Sorry this should be !~. ;)

      Sorry this should be `!~`. ;)
    • Elan Ruusamäe @glensc commented Oct 06, 2016
      1

      well. yes. but that decreases code readability

      well. yes. but that decreases code readability
    Please register or sign in to reply
  • Rémy Coutable @rymai commented Oct 06, 2016
    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?

    @glensc Thanks for your contribution! @dblessing Would you mind just giving your thoughts on this since you've done the original custom hooks implementation?
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • caffdeb3 - filter names and executable in single iteration
    Oct 06, 2016

    Added 1 commit:

    • caffdeb3 - filter names and executable in single iteration
    Added 1 commit: * caffdeb3 - filter names and executable in single iteration
    Toggle commit list
  • Elan Ruusamäe @glensc

    Resolved all discussions

    Oct 06, 2016

    Resolved all discussions

    Resolved all discussions
    Toggle commit list
  • Rémy Coutable @rymai commented Oct 06, 2016
    Master

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

    @glensc Thanks! LGTM, let's wait for @dblessing's feedback! :)
  • Drew Blessing @dblessing commented Oct 06, 2016
    Master

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

    @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. :+1:
  • Drew Blessing @dblessing commented Oct 06, 2016
    Master

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

    As a follow up, we need to update docs in `gitlab-ce/docs`. @glensc would you like to do that?
  • Elan Ruusamäe @glensc commented Oct 06, 2016
    1

    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/*

    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/*`
  • Elan Ruusamäe @glensc commented Oct 06, 2016
    1

    great to see this is getting accepted!

    should i squash commits here and add changelog entry?

    great to see this is getting accepted! should i squash commits here and add changelog entry?
  • Elan Ruusamäe @glensc

    Mentioned in commit glensc/gitlab-ce@c549dda9

    Oct 06, 2016

    Mentioned in commit glensc/gitlab-ce@c549dda9

    Mentioned in commit glensc/gitlab-ce@c549dda9991a79501b0c3222e30b70c7474e94a0
    Toggle commit list
  • Elan Ruusamäe @glensc

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

    Oct 06, 2016

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

    Mentioned in merge request gitlab-org/gitlab-ce!6721
    Toggle commit list
  • Elan Ruusamäe @glensc commented Oct 06, 2016
    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.

    Edited Oct 06, 2016 by Elan Ruusamäe
    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.
  • Rémy Coutable @rymai commented Oct 07, 2016
    Master

    Thanks @glensc! Could you add tests, though?

    Thanks @glensc! Could you add tests, though?
  • Elan Ruusamäe @glensc commented Oct 07, 2016
    1

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

    Adding tests takes some time, rspec (and Ruby) kind a new to me
  • Rémy Coutable @rymai commented Oct 10, 2016
    Master

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

    @glensc No problem, don't hesitate to ask if you need any help!
  • Elan Ruusamäe @glensc commented Oct 10, 2016
    1

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

    I have not started yet, so i do not have questions, but if you have good examples, share them :)
  • Elan Ruusamäe @glensc commented Oct 10, 2016
    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

    Edited Oct 10, 2016 by Elan Ruusamäe
    found similar issue: #32 and it's MR: !89 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
  • Rémy Coutable @rymai commented Oct 11, 2016
    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?

    @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 allows to define global hooks - !93 allows to define multiple hooks of the same type in order to chain them @glensc It seems that !89 is older and mostly ready, so maybe you could rebase upon `master` once it's merged? @dblessing Does it make sense?
  • Elan Ruusamäe @glensc commented Oct 11, 2016
    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>?

    Edited Oct 26, 2016 by Elan Ruusamäe
    @rymai but !89 defines global hooks dir that is somewhat superfluous if !93 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>`?
  • Jacob Vosmaer (GitLab) @jacobvosmaer-gitlab

    Mentioned in merge request !89 (closed)

    Oct 12, 2016

    Mentioned in merge request !89 (closed)

    Mentioned in merge request !89
    Toggle commit list
  • Chris @MrChrisW

    Mentioned in issue #32 (closed)

    Oct 20, 2016

    Mentioned in issue #32 (closed)

    Mentioned in issue #32
    Toggle commit list
  • Elan Ruusamäe @glensc commented Oct 26, 2016
    1

    rebased against current master. altho i'd really prefer rebase against !89 (closed)

    Edited Oct 26, 2016 by Elan Ruusamäe
    rebased against current master. altho i'd really prefer rebase against !89
  • Elan Ruusamäe @glensc

    Added 21 commits:

    • caffdeb3...eae98b67 - 20 commits from branch gitlab-org:master
    • cb6aa61b - Chain custom hooks

    Compare with previous version

    Oct 26, 2016

    Added 21 commits:

    • caffdeb3...eae98b67 - 20 commits from branch gitlab-org:master
    • cb6aa61b - Chain custom hooks

    Compare with previous version

    Added 21 commits: * caffdeb3...eae98b67 - 20 commits from branch `gitlab-org:master` * cb6aa61b - Chain custom hooks [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1263042&start_sha=caffdeb3f061d439b12d213504dbb7187af797b2)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 3 commits:

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

    Compare with previous version

    Oct 29, 2016

    Added 3 commits:

    • cb6aa61b...0f8a3cbb - 2 commits from branch gitlab-org:master
    • 28fc0f5c - 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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1286844&start_sha=cb6aa61ba8311b1599749e02eb91017c006860cd)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 2 commits:

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

    Compare with previous version

    Nov 02, 2016

    Added 2 commits:

    • ed59fbc3 - 1 commit from branch gitlab-org:master
    • 8845fc26 - 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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1319651&start_sha=28fc0f5c4acc0821849ff94bf5f07ac26fe1f876)
    Toggle commit list
  • Elan Ruusamäe @glensc commented Nov 09, 2016
    1

    ok, as i see no movement in either direction, and nobody answering direction either,

    i've rebased my MR !89 (closed)

    ok, as i see no movement in either direction, and nobody answering direction either, i've rebased my MR !89
  • Elan Ruusamäe @glensc

    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

    Nov 09, 2016

    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 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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1402049&start_sha=8845fc26878ca50c2438ce95f8e0dbef94c6086c)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 995b8f6e - updated tests to match current code

    Compare with previous version

    Nov 09, 2016

    Added 1 commit:

    • 995b8f6e - updated tests to match current code

    Compare with previous version

    Added 1 commit: * 995b8f6e - updated tests to match current code [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1403103&start_sha=82e3d76bfecfb47f4894e47465b35342ebef7202)
    Toggle commit list
  • Elan Ruusamäe @glensc commented Nov 09, 2016
    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.

    Edited Nov 09, 2016 by Elan Ruusamäe
    i've updated rspec tests to match this MR changes, and noticed that hooks order differs with the one from !89. 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.
  • Sean McGivern @smcgivern commented Nov 11, 2016
    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 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 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 :+1: > 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!
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Elan Ruusamäe Nov 16, 2016
    CHANGELOG
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      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 😕

      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 :confused:
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

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

      this came from master branch. i think rebase again with master will solve this bogus commit.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      👍

      :+1:
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Elan Ruusamäe Nov 25, 2016
    CHANGELOG
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      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.

      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.
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

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

      will do, @dirker's code is underneath my commits :)
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

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

      added, wasn't sure how. as this is first entry that gets credited in changelog file
    • Sean McGivern @smcgivern commented Nov 17, 2016
      Master

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

      Thanks! There are credited entries further down (https://gitlab.com/gitlab-org/gitlab-shell/blob/master/CHANGELOG#L63), but that's exactly right :+1:
    • Elan Ruusamäe @glensc commented Nov 17, 2016
      1

      On 17.11.2016 12:57, Sean McGivern wrote:

      On 17.11.2016 12:57, Sean McGivern wrote:
    • Elan Ruusamäe @glensc commented Nov 23, 2016
      1

      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!

      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!
    • Sean McGivern @smcgivern commented Nov 23, 2016
      Master

      Touché, fair point! 😀

      Touché, fair point! :grinning:
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Elan Ruusamäe Nov 16, 2016
    hooks/post-receive
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      Isn't this required in `gitlab_post_receive`? Why do we need it here?
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

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

      it came with !89, will check their discussion if any
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      Thanks 👍

      Thanks :+1:
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

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

      it was added to have `ROOT_PATH` symbol available; this is no longer needed. removed in 2a04017
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    lib/gitlab_custom_hook.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      `Dir["#{path}/[^~]*"]` might work?
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

      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.

      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.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      OK, can we use [`String#start_with?`](http://ruby-doc.org/core-2.0.0/String.html#method-i-start_with-3F) in that case please?
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      changed in f253a00a.

      Edited Nov 16, 2016 by Elan Ruusamäe
      changed in f253a00.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    lib/gitlab_custom_hook.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      Let's just make `match_hook_files` return an empty array if the path doesn't exist, and then we can remove this duplication.
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      changed in 5d65423e.

      Edited Nov 16, 2016 by Elan Ruusamäe
      changed in 5d65423.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      I don't think we need this method, maybe just a comment? :slight_smile:
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      6f3305a9.

      Edited Nov 16, 2016 by Elan Ruusamäe
      6f3305a.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      ok -> successful, here and below

      ok -> successful, here and below
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      c63e9ca2.

      Edited Nov 16, 2016 by Elan Ruusamäe
      c63e9ca.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Elan Ruusamäe Nov 25, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      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
      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 >>>
    • Elan Ruusamäe @glensc commented Nov 20, 2016
      1

      how do you suggest to test sorting?

      how do you suggest to test sorting?
    • Sean McGivern @smcgivern commented Nov 23, 2016
      Master

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

      Can we symlink the hooks to names like that, and test the same way as we test the ordering otherwise?
    • Elan Ruusamäe @glensc commented Nov 23, 2016
      1

      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)

      Edited Nov 23, 2016 by Elan Ruusamäe
      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: https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93#note_18695337
    • Sean McGivern @smcgivern commented Nov 23, 2016
      Master

      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.
      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`](https://relishapp.com/rspec/rspec-mocks/docs/setting-constraints/message-order) to the message expecations.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Elan Ruusamäe Nov 25, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      Can we move this methods to above the specs, please? I tend to read specs top-to-bottom.
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      40f97c7f

      40f97c7
    • Elan Ruusamäe @glensc commented Nov 23, 2016
      1

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

      btw, i personally prefer not to see those helpers at first sight. like private methods are defined bottom of the class.
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on an old version of the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      Can we DRY up these a little bit, please? Maybe use a helper method?
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

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

      i've just updated paths from !89, tried to change less as possible
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

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

      i don't know what you have in mind here. too much abstraction breaks readability as well.
    • Sean McGivern @smcgivern commented Nov 17, 2016
      Master

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

      Actually, you're right. Now the rest of the specs are moved around, I think this is fine :+1:
    Please register or sign in to reply
  • Sean McGivern
    @smcgivern started a discussion on the diff Nov 11, 2016
    Resolved by Sean McGivern Nov 17, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

      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 👍

      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 :+1:
    • Elan Ruusamäe @glensc commented Nov 11, 2016
      1

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

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

      i'm new to ruby and rspec, what is helper? :) you mean add some class to `spec/spec_helper.rb` ?
    • Sean McGivern @smcgivern commented Nov 11, 2016
      Master

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

      Just a helper method, like you have further down with `def`. Nothing more complicated than that :smiley:
    • Elan Ruusamäe @glensc commented Nov 16, 2016
      1

      9db5caa8

      9db5caa
    Please register or sign in to reply
  • Sean McGivern @smcgivern commented Nov 11, 2016
    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! 🌠

    @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! :stars:
  • Sean McGivern @smcgivern

    Reassigned to @glensc

    Nov 11, 2016

    Reassigned to @glensc

    Reassigned to @glensc
    Toggle commit list
  • Sean McGivern @smcgivern

    Milestone changed to 8.15

    Nov 15, 2016

    Milestone changed to 8.15

    Milestone changed to %11
    Toggle commit list
  • Elan Ruusamäe @glensc

    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

    Nov 15, 2016

    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

    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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1470549&start_sha=995b8f6e78f4f766f9cfd436d89b1b7663360fa2)
    Toggle commit list
  • Elan Ruusamäe @glensc commented Nov 15, 2016
    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

    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
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 2a040173 - remove no longer needed gitlab_init

    Compare with previous version

    Nov 16, 2016

    Added 1 commit:

    • 2a040173 - remove no longer needed gitlab_init

    Compare with previous version

    Added 1 commit: * 2a040173 - remove no longer needed gitlab_init [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1470706&start_sha=0fa85f9159dc90369d87bca831adbcdb83a911ca)
    Toggle commit list
  • Elan Ruusamäe @glensc

    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

    Nov 16, 2016

    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: * 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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1470837&start_sha=2a04017383328c7daa361e88b69d4985efcee963)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 2 commits:

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

    Compare with previous version

    Nov 16, 2016

    Added 2 commits:

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

    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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1470848&start_sha=5d65423e75e481f87dbbe1fedaef103858845709)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • f822ccca - changelog entry

    Compare with previous version

    Nov 16, 2016

    Added 1 commit:

    • f822ccca - changelog entry

    Compare with previous version

    Added 1 commit: * f822ccca - changelog entry [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1482557&start_sha=c63e9ca23c436b0e69bfb8a80afe559dfaa00284)
    Toggle commit list
  • Dirk Hörner @dirker commented Nov 16, 2016

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

    Hi @glensc, just wanted to say thanks for following through so this feature can finally get merged :thumbsup:. @smcgivern thanks for reviewing!
  • Elan Ruusamäe @glensc commented Nov 16, 2016
    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.

    @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.
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 40f97c7f - move helpers to top

    Compare with previous version

    Nov 16, 2016

    Added 1 commit:

    • 40f97c7f - move helpers to top

    Compare with previous version

    Added 1 commit: * 40f97c7f - move helpers to top [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1484095&start_sha=f822ccca8156b8bcfe809fa7eeac00dd27853088)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Compare with previous version

    Nov 16, 2016

    Added 1 commit:

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

    Compare with previous version

    Added 1 commit: * 9db5caa8 - cleanup dirs in before to fixup aborted tests [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1484144&start_sha=40f97c7febcab14da1534a1d27f1ee94c9ee3319)
    Toggle commit list
  • Elan Ruusamäe @glensc commented Nov 17, 2016
    1

    also who should mark "discussion resolved"?

    also who should mark "discussion resolved"?
  • Elan Ruusamäe @glensc commented Nov 17, 2016
    1

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

    and perhaps merge without further tests, it seems accepted here: !106 which actually broke something: #70
  • Sean McGivern
    @smcgivern started a discussion on the diff Nov 17, 2016
    Resolved by Elan Ruusamäe Nov 25, 2016
    spec/gitlab_custom_hook_spec.rb
    Unable to load the diff.
    • Sean McGivern @smcgivern commented Nov 17, 2016
      Master

      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)>'
      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)>' ```
    • Elan Ruusamäe @glensc commented Nov 17, 2016
      1

      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.

      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.
    • Elan Ruusamäe @glensc commented Nov 20, 2016
      1

      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

      i've fixed `GL_ID` test locally (866c8ee), 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
    • Elan Ruusamäe @glensc commented Nov 20, 2016
      1

      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.

      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.
    • Elan Ruusamäe @glensc commented Nov 20, 2016
      1

      EDIT: probably fixed in 30bda7eb

      EDIT: probably fixed in 30bda7e
    • Sean McGivern @smcgivern commented Nov 23, 2016
      Master

      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.

      > 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 `context`s for 'with local hooks', 'with global hooks', and just do the different `before`s in there.
    Please register or sign in to reply
  • Sean McGivern @smcgivern commented Nov 17, 2016
    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!

    @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 :heart: 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 which actually broke something: #70 Right, but we shouldn't have done that :wink: I still need to test this manually as well, once it's green on CI and ready to merge!
  • Elan Ruusamäe @glensc

    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

    Nov 20, 2016

    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 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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1518381&start_sha=9db5caa8609d200724ffa78b2b8f69f13e2b74b7)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • 866c8ee7 - fix gl_id_test_hook

    Compare with previous version

    Nov 20, 2016

    Added 1 commit:

    • 866c8ee7 - fix gl_id_test_hook

    Compare with previous version

    Added 1 commit: * 866c8ee7 - fix gl_id_test_hook [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1519337&start_sha=47b3a19233566cf77b7fe012189c0be77080c02e)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Compare with previous version

    Nov 20, 2016

    Added 1 commit:

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

    Compare with previous version

    Added 1 commit: * c2f4a9da - spec&#x2F;custom_hooks: cleanup helpers not to repeat repo path parameter [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1519426&start_sha=866c8ee717fe364214d4e6c054d0be7332dd4642)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

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

    Compare with previous version

    Nov 20, 2016

    Added 1 commit:

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

    Compare with previous version

    Added 1 commit: * 30bda7eb - spec&#x2F;custom_hook: ensure &quot;before&quot; does complete cleanup [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1519689&start_sha=c2f4a9da81047c6e6f8128a4de15133c3ff79961)
    Toggle commit list
  • Sean McGivern @smcgivern commented Nov 23, 2016
    Master

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

    @glensc thanks! I think I just have one comment left.
  • Elan Ruusamäe @glensc

    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

    Nov 25, 2016

    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

    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](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1592357&start_sha=30bda7eb3b7dd1d5ed08473de2b1a7e8bc08ce64)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Resolved all discussions

    Nov 25, 2016

    Resolved all discussions

    Resolved all discussions
    Toggle commit list
  • Elan Ruusamäe @glensc commented Nov 25, 2016
    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 :)

    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 :)
  • Elan Ruusamäe @glensc

    Changed title: chain custom hooks in <hook_name>.d dirs → Add support for global custom hooks and chained hook directories

    Nov 25, 2016

    Changed title: chain custom hooks in <hook_name>.d dirs → Add support for global custom hooks and chained hook directories

    Changed title: **{-chain custom hooks in &lt;hook_name&gt;.d dir-}s** → **{+Add support for global custom hooks and chained hook directorie+}s**
    Toggle commit list
  • Elan Ruusamäe @glensc

    Added 1 commit:

    • b28718cf - changelog spelling

    Compare with previous version

    Nov 25, 2016

    Added 1 commit:

    • b28718cf - changelog spelling

    Compare with previous version

    Added 1 commit: * b28718cf - changelog spelling [Compare with previous version](https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/93/diffs?diff_id=1592413&start_sha=bda3016edf3d890318e0c060cd10ef36d52d2c6a)
    Toggle commit list
  • Elan Ruusamäe @glensc

    Mentioned in commit glensc/gitlab-ce@896eae05

    Nov 25, 2016

    Mentioned in commit glensc/gitlab-ce@896eae05

    Mentioned in commit glensc/gitlab-ce@896eae05f041cf4f63e165763a4b49eb5e7774b8
    Toggle commit list
  • Elan Ruusamäe @glensc

    Mentioned in commit glensc/gitlab-ce@2091b12a

    Nov 25, 2016

    Mentioned in commit glensc/gitlab-ce@2091b12a

    Mentioned in commit glensc/gitlab-ce@2091b12a15c24ba1621f3bc405a4212905f32177
    Toggle commit list
  • Drew Blessing @dblessing

    Reassigned to @smcgivern

    Nov 28, 2016

    Reassigned to @smcgivern

    Reassigned to @smcgivern
    Toggle commit list
  • Sean McGivern @smcgivern commented Dec 01, 2016
    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!

    @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!
  • Sean McGivern @smcgivern

    Status changed to closed

    Dec 01, 2016

    Status changed to closed

    Status changed to closed
    Toggle commit list
  • Elan Ruusamäe @glensc commented Dec 03, 2016
    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.

    @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.
  • Sean McGivern @smcgivern commented Dec 03, 2016
    Master

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

    @glensc I did rebase! I just can't push to your fork :slight_smile:
  • Elan Ruusamäe @glensc commented Dec 04, 2016
    1

    the new MR is at !111 (merged)

    the new MR is at !111
  • Elan Ruusamäe @glensc

    mentioned in commit glensc/gitlab-ce@e7353406

    Dec 04, 2016

    mentioned in commit glensc/gitlab-ce@e7353406

    mentioned in commit glensc/gitlab-ce@e7353406f15bdbab7e677f18a4886fb58f9464a1
    Toggle commit list
  • Elan Ruusamäe @glensc

    mentioned in commit glensc/gitlab-ce@bc380e48

    Dec 06, 2016

    mentioned in commit glensc/gitlab-ce@bc380e48

    mentioned in commit glensc/gitlab-ce@bc380e48486903637ec9da977ad2d5bab3dc9dfd
    Toggle commit list
  • Adam Niedzielski @adamniedzielski

    mentioned in issue gitlab-ce#14243 (closed)

    Jan 03, 2017

    mentioned in issue gitlab-ce#14243 (closed)

    mentioned in issue gitlab-ce#14243
    Toggle commit list
  • Write
  • Preview
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment
Sean McGivern
Assignee
Sean McGivern @smcgivern
Assign to
8.15
Milestone
8.15
Assign milestone
Time tracking
0
Labels
None
Assign labels
  • View project labels
Reference: gitlab-org/gitlab-shell!93