Add gitaly notification on post-receive hook

This is the gitlab-shell part of gitaly#37 (closed)

mentioned in merge request !118 (closed)

Looks good. Wondering how we should test.

I think the next step here is to add tests.

assigned to @eReGeBe

added 1 commit

• b98d59ac - Add gitaly notification on post-receive hook

Compare with previous version

assigned to @jacobvosmaer-gitlab

OK so to have proper tests with a VCR recording we have a bit of work to do:

• merge https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8983
• wait for it to deploy onto dev.gitlab.org
• install a pre-release version of gitaly on dev.gitlab.org
• record the request

That sounds unpractical. @pcarranza git log says you are a recent vcr cassette committer; how did you deal with this?

I am strongly inclined to fake it with localhost and hand-edit the vcr cassette before committing.

added 1 commit

• 6bc3b925 - Use api/v4 for gitaly notification and add tests

Compare with previous version

@jacobvosmaer-gitlab I think faking it with localhost is correct, those cassettes had to be edited anyways to remove the real secret token from dev.gitlab.org. That also allowed me to realize I need to use api/v4 for this endpoint. Check the latest commit.

mentioned in issue gitaly#37 (closed)

resolved all discussions

Looks good!

added 1 commit

• 09e48144 - Use api/v4 for gitaly notification and add tests

Compare with previous version

resolved all discussions

@eReGeBe please pick a maintainer to have this reviewed and merged. No further comments from me at this time.

assigned to @eReGeBe

added Gitaly label

@rspeicher could you take a look at this one?

assigned to @rspeicher

@eReGeBe @jacobvosmaer-gitlab Is this meant for 8.17 or 9.0?

@eReGeBe One note.

assigned to @eReGeBe

@rspeicher it'd be nice to have this in 8.17 because it would allow us to do more experimenting. It would be experimental-only. Does that impact the timing of when to merge this?

Whether having this in 8.17 is realistic depends on us getting https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8983 in too.

resolved all discussions

@jacobvosmaer-gitlab Works for me. I'll merge this now and give it %9.0, but if we get the MR you referenced in for %8.17 we can bump the shell version and include it.

changed milestone to %9.0

assigned to @rspeicher

mentioned in commit a6e5b9a1

merged

Hmm now what happens if we bump gitlab-shell in gitlab-ce before https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8983 is merged. @eReGeBe will we have a problem then? What happens if gitlab-shell master talkes to gitlab-ce api and gitlab-ce does not have this endpoint yet?

@jacobvosmaer-gitlab just tried locally. You get a line like this one in gitlab-shell.log: E, [2017-02-17T17:11:25.518180 #17690] ERROR -- : API call <POST http+unix://%2FUsers%2Falejandro%2Fgdk%2Fgitlab.socket/api/v4/internal/notify_post_receive> failed: 404 => <{"error":"404 Not Found"}>., but the push succeeds and no errors are communicated to the user, so we should be fine.

Fine-ish. People out there do watch those logs and this will lead to questions. :) But the most important thing is that users are not bothered.