Repository navigation
Add DeliveryID function for webhook events - #619
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
44a2181 to
8fab24e
Compare
|
I was using the wrong git config, I've addressed the googlebot's issue above. |
|
CLAs look good, thanks! |
| return r.Header.Get(eventTypeHeader) | ||
| } | ||
|
|
||
| // DeliveryID returns the unique delivery ID of webhook request r. |
There was a problem hiding this comment.
Perhaps it's a good idea to link to the relevant GitHub API docs here?
// DeliveryID returns the unique delivery ID of webhook request r.
//
// GitHub API docs: https://developer.github.com/v3/repos/hooks/#webhook-headers
If you want to, you can add the same to docs of WebHookType.
|
|
||
| got := DeliveryID(req) | ||
| if got != id { | ||
| t.Errorf("DeliveryID(%#v) = %#v, want %#v", req, got, id) |
There was a problem hiding this comment.
nit: since got and id are both strings, you may want to use %q for these values instead of %#v.
t.Errorf("DeliveryID(%#v) = %q, want %q", req, got, id)
There was a problem hiding this comment.
Drew, the commit message of ffae808 is "Fix formatting of test output". However, both %#v and %q produce same results for string type. It wasn't wrong, this is a mere style change. Just wanted to point that out FYI.
There was a problem hiding this comment.
Yeah, sorry, bad wording. I meant "fixed" in the context of "for the review". Would you like me to change the commit message to be more precise?
There was a problem hiding this comment.
No problem. No need to change the commit message, this PR will be squashed into 1 commit when merging anyway, and the review-specific parts don't need to be included.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
LGTM. Thank you, @drewfradette and @shurcooL!
|
Thanks @drewfradette! |
Addresses #618