Add support for listing and getting repository/organization webhook deliveries #1934
Conversation
google-cla
bot
added
the
cla: yes
| return h, resp, nil | ||
| } | ||
|
|
||
| func (d *HookDelivery) ParseRequestPayload() (interface{}, error) { |
There was a problem hiding this comment.
This is a utility function to ease parsing event payloads, and it is derived from the equivalent function for Events API
Lines 29 to 31 in 05e95d3
There was a problem hiding this comment.
Why can't you reuse https://github.com/google/go-github/blob/master/github/event.go#L29-L132 here?
There was a problem hiding this comment.
@gmlewis Hey! Interesting idea. How would you like to do that? That snippet in event.go switch on Type whose the value can be e.g. CheckRunEvent. On the other hand, this one switches on event field where the value is e.g. check_run. They're very similar but different enough to worth a dedicated function.
There was a problem hiding this comment.
Also note that this is almost fully covered by TestHookDelivery_ParsePayload, if that helps.
There was a problem hiding this comment.
What I'm thinking here is to make a helper function that both ParsePayload and ParseRequestPayload both call.
The helper method could take an anonymous function to perform the required mapping... one of them would use the eventTypeMethod map found in messages.go.
Let me know if you don't think this is possible and I'll download your PR and see if I can get it to work.
There was a problem hiding this comment.
Here's my proposed replacement for this method:
// ParseRequestPayload parses the request payload. For recognized event types,
// a value of the corresponding struct type will be returned.
func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {
eType, ok := eventTypeMapping[*d.Event]
if !ok {
return nil, fmt.Errorf("unsupported event type %q", *d.Event)
}
e := &Event{Type: &eType, RawPayload: d.Request.RawPayload}
return e.ParsePayload()
}
Codecov Report
@@ Coverage Diff @@
## master #1934 +/- ##
==========================================
+ Coverage 97.65% 97.82% +0.17%
==========================================
Files 105 107 +2
Lines 6786 6865 +79
==========================================
+ Hits 6627 6716 +89
+ Misses 86 82 -4
+ Partials 73 67 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you, @mumoshu.
I'm thinking that this can be improved by reusing more of the existing event code or refactoring it so that it is easier to reuse.
Also, the drop in code coverage is a bit concerning, but this might get fixed by reducing the amount of new code and reusing more of the existing code.
| @@ -481,25 +489,33 @@ func (r *Response) populatePageValues() { | |||
| if err != nil { | |||
| continue | |||
| } | |||
| page := url.Query().Get("page") | |||
| if page == "" { | |||
|
|
|||
There was a problem hiding this comment.
Hmmm... I'm not sure I'm happy with this section (especially regarding increasing the already-deep indentation levels).
Since "cursor" is the less-common and smaller case, let's please handle it first, and let's revert all the changes made to the handling of the "page" section, except maybe to pull out the shared query first... something like this:
q := url.Query()
if cursor := q.Get("cursor"); cursor != "" {
...
}
page := q.Get("page")
if page == "" {
continue
}
...There was a problem hiding this comment.
Thanks! Addressed in f6d688a
github/repos_hooks_deliveries.go
Outdated
| type HookDelivery struct { | ||
| ID *int64 `json:"id"` | ||
| GUID *string `json:"guid"` | ||
| DeliveredAt *time.Time `json:"delivered_at"` |
There was a problem hiding this comment.
| DeliveredAt *time.Time `json:"delivered_at"` | |
| DeliveredAt *Timestamp `json:"delivered_at"` |
There was a problem hiding this comment.
Addressed in ff5a47a !
github/repos_hooks_deliveries.go
Outdated
| } | ||
|
|
||
| type HookResponse struct { | ||
| Header map[string]string `json:"headers,omitempty"` |
There was a problem hiding this comment.
| Header map[string]string `json:"headers,omitempty"` | |
| Headers map[string]string `json:"headers,omitempty"` |
github/repos_hooks_deliveries.go
Outdated
| // | ||
| // GitHub API docs: https://docs.github.com/en/rest/reference/repos#list-deliveries-for-a-repository-webhook | ||
| func (s *RepositoriesService) ListHookDeliveries(ctx context.Context, owner, repo string, id int64, opts *ListCursorOptions) ([]*HookDelivery, *Response, error) { | ||
| u := fmt.Sprintf("repos/%v/%v/hooks/%d/deliveries", owner, repo, id) |
There was a problem hiding this comment.
| u := fmt.Sprintf("repos/%v/%v/hooks/%d/deliveries", owner, repo, id) | |
| u := fmt.Sprintf("repos/%v/%v/hooks/%v/deliveries", owner, repo, id) |
There was a problem hiding this comment.
Fine! But out of curiosity, why do you prefer using %v for an integer value?
There was a problem hiding this comment.
Fixed in 5f0e1e4 anyway
There was a problem hiding this comment.
I prefer %v for the following reasons:
- more consistent with the rest of the repo,
- idiomatic Go,
- and most importantly, it makes it much easier to search for an existing endpoint when you don't have to figure out the types of the segments of the URLs... you can more easily transform a URL from the documentation into a search string into the code base.
There was a problem hiding this comment.
Gotcha! That totally makes sense. Thanks a lot for clarifying.
Regarding consistency with the rest of the repo, JFYI, I found a bunch of %d occurrences in the existing sources like repos_hook.go. Probably you'd want to fix those in another circumstance.
| return h, resp, nil | ||
| } | ||
|
|
||
| func (d *HookDelivery) ParseRequestPayload() (interface{}, error) { |
There was a problem hiding this comment.
Why can't you reuse https://github.com/google/go-github/blob/master/github/event.go#L29-L132 here?
|
Ok |
|
@gmlewis Hey! I've addressed all the comments. Would you mind reviewing once again? Also, please feel free to squash on merge, or just tell me to do so, if necessary. Thanks! |
|
FYI: I've added 431c77c for improving test coverage |
|
I've added 23922a8 for more code coverage on All the new codes should have been covered. I have no immediate plan to add more commits now so it should be a good timing for you to review again 😄 |
|
Apparently, the last commit 23922a8 is not included in https://app.codecov.io/gh/google/go-github/branch/webhook-deliveries-api yet. But theoretically speaking the code coverage shouldn't get decreased now, as I've already added enough tests to cover all the new code paths. |
We always squash and merge in this repo, so don't worry about the number of commits. In fact, we prefer that author's don't force-push so that it is easier for reviewers to see what changes have been made since the last review. |
| r.Cursor = cursor | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Are "cursor" and "page" mutually exclusive? (I think the answer is "yes" but I could be wrong.)
If so, do you want to add a continue within this if block just to make that clear?
I'm actually fine either way... your call.
There was a problem hiding this comment.
Thanks! Yes, I believe so, although it isn't documented anywhere. Added continue in 72c3380
github/repos_hooks_deliveries.go
Outdated
| InstallationID *string `json:"installation_id"` | ||
| RepositoryID *int64 `json:"repository_id"` | ||
|
|
||
| // Request is populated by GetHookDelivery |
There was a problem hiding this comment.
nit:
| // Request is populated by GetHookDelivery | |
| // Request is populated by GetHookDelivery. |
There was a problem hiding this comment.
Addressed in 5b86aff 👍
github/repos_hooks_deliveries.go
Outdated
|
|
||
| // Request is populated by GetHookDelivery | ||
| Request *HookRequest `json:"request,omitempty"` | ||
| // Response is populated by GetHookDelivery |
There was a problem hiding this comment.
nit:
| // Response is populated by GetHookDelivery | |
| // Response is populated by GetHookDelivery. |
There was a problem hiding this comment.
Addressed in 5b86aff 👍
| return Stringify(d) | ||
| } | ||
|
|
||
| type HookRequest struct { |
There was a problem hiding this comment.
Can you please add a comment here describing // HookRequest ... ?
| return Stringify(r) | ||
| } | ||
|
|
||
| type HookResponse struct { |
There was a problem hiding this comment.
Can you please add a comment here describing // HookResponse ... ?
| return h, resp, nil | ||
| } | ||
|
|
||
| func (d *HookDelivery) ParseRequestPayload() (interface{}, error) { |
There was a problem hiding this comment.
What I'm thinking here is to make a helper function that both ParsePayload and ParseRequestPayload both call.
The helper method could take an anonymous function to perform the required mapping... one of them would use the eventTypeMethod map found in messages.go.
Let me know if you don't think this is possible and I'll download your PR and see if I can get it to work.
Sounds good!
And would you mind if I also created another, reverse map of |
|
Apparently, another method named Perhaps we'd better revise |
OK. I'll take a look and get back to you. |
I think I've got a simple solution for you. It also caught 3 errors in your test table, which I think justifies the reuse of the existing code. I'll add review comments so you can see what I did. Let me know what you think. |
There was a problem hiding this comment.
Hi @mumoshu - here are my suggestions for reusing existing code, along with a few error corrections. Please let me know if they make sense to you.
Please remember to run gofmt after accepting my suggestions, as they will need proper formating.
| return h, resp, nil | ||
| } | ||
|
|
||
| func (d *HookDelivery) ParseRequestPayload() (interface{}, error) { |
There was a problem hiding this comment.
Here's my proposed replacement for this method:
// ParseRequestPayload parses the request payload. For recognized event types,
// a value of the corresponding struct type will be returned.
func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {
eType, ok := eventTypeMapping[*d.Event]
if !ok {
return nil, fmt.Errorf("unsupported event type %q", *d.Event)
}
e := &Event{Type: &eType, RawPayload: d.Request.RawPayload}
return e.ParsePayload()
}
|
@gmlewis Thank you so much for taking your time and the suggestions! All looked awesome and I've included your suggestions in the last commit, 1381c0f. Regarding the change on |
|
@gmlewis Hey! Would you mind if I added another commit for adding similar support for organization variants of these APIs? It already has full test coverage, and I've manually verified it to work as part of actions/actions-runner-controller#682, too. @Parker77 Hi! If you're reading this and going to put the second LGTM, please go ahead! I'm just trying to reduce the time required on both ends to make the set of changes related to #1933. If you merged this, I'll just submit another PR for mumoshu@0a3739c. |
mumoshu
changed the title
Resolves #1933
To make this PR minimal as possible for ease of reviewing, I intentionally included only four APIs related to the webhook deliveries API:
That being said, the following methods aren't covered in this PR. If you like, I'd gladly submit subsequent PRs to add all of them.
List/get/redelivery for organizational webhooksNote that changes on the following files are completely generated by running
go generateas documented in your guide, so you won't need to read them that carefully.github/github-accessors_test.gogithub/github-accessors.goFWIW, you can see how these additional methods can be used to retrieve historical webhook deliveries here:
https://github.com/actions-runner-controller/actions-runner-controller/blob/c3cfab81c85d33c65f7b6f6443da0d2f57fd72db/pkg/githubwebhookdeliveryforwarder/githubwebhookdelivery.go#L96-L139
The part that propagates
Cursorfrom the response to the ListCursorOptions for the next paginated request is beneficial to understanding how the cursor can be used for pagination.