Add APIs for pre-receive hooks #945
Conversation
googlebot
added
the
cla: yes
github/repos_prereceive_hooks.go
Outdated
| @@ -0,0 +1,102 @@ | |||
| // Copyright 2013 The go-github AUTHORS. All rights reserved. | |||
There was a problem hiding this comment.
s/2013/2018/ as that is the year this particular file is created.
github/repos_prereceive_hooks.go
Outdated
| ConfigURL *string `json:"configuration_url,omitempty"` | ||
| } | ||
|
|
||
| func (h PreReceiveHook) String() string { |
There was a problem hiding this comment.
Go style nit: func (p PreReceiveHook)...
s/h/p/ to match the receiver variable with the start of the name of its type...
although I see that this Go style recommendation is not followed consistently throughout this repo, so you can ignore this comment safely. I'll leave it anyway in case someone wants to clean up all receivers in this repo. 😄
github/repos_prereceive_hooks.go
Outdated
|
|
||
| hook := new(PreReceiveHook) | ||
| resp, err := s.client.Do(ctx, req, hook) | ||
| return hook, resp, err |
There was a problem hiding this comment.
I think we would like the error checking to remain consistent, like your function above:
if err != nil {
return nil, resp, err
}
return hook, resp, nilWe think this is easier to read and reason about.
If you copied this pattern from somewhere else in this repo, we should probably change that one too.
There was a problem hiding this comment.
Yes, this file was a copy-modify of repos_hooks.go, so these issues are also copied from there. Will make the changes in this PR and open a separate PR to fix the occurrences in repos_hooks.go.
github/repos_prereceive_hooks.go
Outdated
|
|
||
| h := new(PreReceiveHook) | ||
| resp, err := s.client.Do(ctx, req, h) | ||
| return h, resp, err |
There was a problem hiding this comment.
same here with the error handling
| // TODO: remove custom Accept header when this API fully launches. | ||
| req.Header.Set("Accept", mediaTypePreReceiveHooksPreview) | ||
|
|
||
| return s.client.Do(ctx, req, nil) |
There was a problem hiding this comment.
This is fine to leave as-is since we are not returning any "interesting" struct pointers here.
| @@ -0,0 +1,133 @@ | |||
| // Copyright 2013 The go-github AUTHORS. All rights reserved. | |||
| defer teardown() | ||
|
|
||
| mux.HandleFunc("/repos/o/r/pre-receive-hooks", func(w http.ResponseWriter, r *http.Request) { | ||
| testMethod(t, r, "GET") |
There was a problem hiding this comment.
Here and below, please add testHeader to check for the custom media type specified in the header.
That helps us to keep this repo cleaner by explicitly calling out when custom media headers are being used.
nmiyake
force-pushed
the
addPreReceiveHook
branch
from
bfc69c5
to
a704845
Compare
|
Thanks for the review! Updated PR to address feedback. |
nmiyake
force-pushed
the
addPreReceiveHook
branch
from
a704845
to
85b31fa
Compare
|
Thanks for the changes, @nmiyake. Just FYI... it makes reviewing easier if you add additional commits (instead of force-pushing over top of an earlier commit). That way, reviewers can see just the changes that were made after the last review. In the end, we squash and merge the final commit anyway, so feel free to add as many commits as you want. 😄 |
nmiyake
force-pushed
the
addPreReceiveHook
branch
from
85b31fa
to
138faba
Compare
|
Update PR to resolve merge conflict. Ping for another reviewer (@juliaferraioli or anyone else actively doing reviews) to unblock. |
|
Thank you, @nmiyake and @juliaferraioli! |
Fixes #944