Add support for listing and getting repository/organization webhook deliveries #1934

mumoshu · 2021-07-06T02:32:05Z

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:

Listing repository hook deliveries
Getting a specific repository hook delivery
Listing organization hook deliveries
GEtting a specific organization hook delivery

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.

Trigger redelivery of a hook delivery
~~List/get/redelivery for organizational webhooks~~

Note that changes on the following files are completely generated by running go generate as documented in your guide, so you won't need to read them that carefully.

github/github-accessors_test.go
github/github-accessors.go

FWIW, 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 Cursor from the response to the ListCursorOptions for the next paginated request is beneficial to understanding how the cursor can be used for pagination.

Resolves google#1933

github/github.go

mumoshu · 2021-07-06T02:43:43Z

github/repos_hooks_deliveries.go

+	return h, resp, nil
+}
+
+func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {


This is a utility function to ease parsing event payloads, and it is derived from the equivalent function for Events API

go-github/github/event.go

Lines 29 to 31 in 05e95d3

// ParsePayload parses the event payload. For recognized event types,

// a value of the corresponding struct type will be returned.

func (e *Event) ParsePayload() (payload interface{}, err error) {

Why can't you reuse https://github.com/google/go-github/blob/master/github/event.go#L29-L132 here?

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

Also note that this is almost fully covered by TestHookDelivery_ParsePayload, if that helps.

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.

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() }

github/repos_hooks_deliveries.go

codecov · 2021-07-06T03:23:15Z

Codecov Report

Merging #1934 (123bafa) into master (05e95d3) will increase coverage by 0.17%.
The diff coverage is 96.49%.

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

Impacted Files	Coverage Δ
github/repos_hooks_deliveries.go	`93.10% <93.10%> (ø)`
github/github.go	`97.07% <100.00%> (+0.06%)`	⬆️
github/orgs_hooks_deliveries.go	`100.00% <100.00%> (ø)`
github/pulls.go	`97.05% <0.00%> (ø)`
github/checks.go	`100.00% <0.00%> (ø)`
github/activity.go	`100.00% <0.00%> (ø)`
github/event_types.go	`100.00% <0.00%> (ø)`
github/code-scanning.go	`100.00% <0.00%> (ø)`
github/issues_timeline.go	`100.00% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05e95d3...123bafa. Read the comment docs.

gmlewis

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.

gmlewis · 2021-07-06T15:10:21Z

github/github.go

@@ -481,25 +489,33 @@ func (r *Response) populatePageValues() {
 			if err != nil {
 				continue
 			}
-			page := url.Query().Get("page")
-			if page == "" {
+


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 } ...

Thanks! Addressed in f6d688a

gmlewis · 2021-07-06T15:11:42Z

github/repos_hooks_deliveries.go

+type HookDelivery struct {
+	ID             *int64     `json:"id"`
+	GUID           *string    `json:"guid"`
+	DeliveredAt    *time.Time `json:"delivered_at"`


Suggested change

DeliveredAt *time.Time `json:"delivered_at"`

DeliveredAt *Timestamp `json:"delivered_at"`

Addressed in ff5a47a !

github/repos_hooks_deliveries.go

gmlewis · 2021-07-06T15:17:09Z

github/repos_hooks_deliveries.go

+}
+
+type HookResponse struct {
+	Header     map[string]string `json:"headers,omitempty"`


Suggested change

Header map[string]string `json:"headers,omitempty"`

Headers map[string]string `json:"headers,omitempty"`

Fixed in dffbb1c

gmlewis · 2021-07-06T15:19:25Z

github/repos_hooks_deliveries.go

+//
+// 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)


Suggested change

u := fmt.Sprintf("repos/%v/%v/hooks/%d/deliveries", owner, repo, id)

u := fmt.Sprintf("repos/%v/%v/hooks/%v/deliveries", owner, repo, id)

Fine! But out of curiosity, why do you prefer using %v for an integer value?

Fixed in 5f0e1e4 anyway

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.

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.

gmlewis · 2021-07-06T15:26:53Z

github/repos_hooks_deliveries.go

+	return h, resp, nil
+}
+
+func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {


Why can't you reuse https://github.com/google/go-github/blob/master/github/event.go#L29-L132 here?

…g json field

ohpintu90 · 2021-07-06T21:55:35Z

Ok

mumoshu · 2021-07-06T21:57:45Z

@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!

mumoshu · 2021-07-06T22:05:27Z

FYI: I've added 431c77c for improving test coverage

mumoshu · 2021-07-06T22:57:33Z

I've added 23922a8 for more code coverage on cursor handling.

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 😄

mumoshu · 2021-07-06T23:24:10Z

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.

gmlewis · 2021-07-07T00:18:19Z

Also, please feel free to squash on merge, or just tell me to do so, if necessary. Thanks!

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.

gmlewis · 2021-07-07T00:21:34Z

github/github.go

+						r.Cursor = cursor
+					}
+				}
+			}


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.

Thanks! Yes, I believe so, although it isn't documented anywhere. Added continue in 72c3380

gmlewis · 2021-07-07T00:22:53Z

github/repos_hooks_deliveries.go

+	InstallationID *string    `json:"installation_id"`
+	RepositoryID   *int64     `json:"repository_id"`
+
+	// Request is populated by GetHookDelivery


nit:

Suggested change

// Request is populated by GetHookDelivery

// Request is populated by GetHookDelivery.

Addressed in 5b86aff 👍

gmlewis · 2021-07-07T00:23:09Z

github/repos_hooks_deliveries.go

+
+	// Request is populated by GetHookDelivery
+	Request *HookRequest `json:"request,omitempty"`
+	// Response is populated by GetHookDelivery


nit:

Suggested change

// Response is populated by GetHookDelivery

// Response is populated by GetHookDelivery.

Addressed in 5b86aff 👍

gmlewis · 2021-07-07T00:24:01Z

github/repos_hooks_deliveries.go

+	return Stringify(d)
+}
+
+type HookRequest struct {


Can you please add a comment here describing // HookRequest ... ?

Added in 194eff6 👍

gmlewis · 2021-07-07T00:24:28Z

github/repos_hooks_deliveries.go

+	return Stringify(r)
+}
+
+type HookResponse struct {


Can you please add a comment here describing // HookResponse ... ?

Added in 194eff6 👍

gmlewis · 2021-07-07T00:32:09Z

github/repos_hooks_deliveries.go

+	return h, resp, nil
+}
+
+func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {


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.

mumoshu · 2021-07-08T11:18:53Z

What I'm thinking here is to make a helper function that both ParsePayload and ParseRequestPayload both call.

Sounds good!

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.

And would you mind if I also created another, reverse map of eventTypeMethod, named like typeEventMapping, and use that from ParsePayload?

mumoshu · 2021-07-08T11:24:24Z

Apparently, another method named ParseWebhook that calls ParsePayload seems to already use eventTypeMapping on its own 🤔 Probably adding second use of eventTypeMapping in an anonymous function defined in ParseWebhook doesn't make much sense?

Perhaps we'd better revise ParseWebHook as well? I'm pretty lost on how I should go.

gmlewis · 2021-07-08T12:07:36Z

I'm pretty lost on how I should go.

OK. I'll take a look and get back to you.

gmlewis · 2021-07-08T14:11:23Z

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.

gmlewis

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.

gmlewis · 2021-07-08T14:15:15Z

github/repos_hooks_deliveries.go

+	return h, resp, nil
+}
+
+func (d *HookDelivery) ParseRequestPayload() (interface{}, error) {


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() }

github/repos_hooks_deliveries_test.go

google#1934 (review)

mumoshu · 2021-07-09T00:05:20Z

@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 ParseRequestPayload, I initially had some preconceived idea that it won't be acceptable to create a new Event object just for calling ParsePayload. Apparently, it was the best impl though. Good job!

gmlewis

Thank you, @mumoshu !
LGTM.

Awaiting second LGTM before merging.

Ref google#1933

mumoshu · 2021-07-12T00:06:06Z

@gmlewis Hey! Would you mind if I added another commit for adding similar support for organization variants of these APIs?

mumoshu@0a3739c.

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.

gmlewis · 2021-07-12T01:40:07Z

@gmlewis Hey! Would you mind if I added another commit for adding similar support for organization variants of these APIs?

Hi @mumoshu - sure, you can add more commits if you wish... please just push and don't use "force push" so that it will be easier to review more recent additions. Thank you.

mumoshu · 2021-07-12T11:16:43Z

@gmlewis Thanks for confirming! I've added 0a3739c and updated the PR description and title a bit to reflect the changes

gmlewis

Thank you, @mumoshu !
Just a couple minor tweaks, please.

github/orgs_hooks_deliveries.go

gmlewis

Thank you, @mumoshu !
LGTM.

Awaiting second LGTM before merging.

Parker77

LGTM.

Add support for listing and getting repository webhook deliveries

54e5502

Resolves google#1933

google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 6, 2021

mumoshu commented Jul 6, 2021

View reviewed changes

github/github.go Show resolved Hide resolved

mumoshu commented Jul 6, 2021

View reviewed changes

github/repos_hooks_deliveries.go Show resolved Hide resolved

mumoshu commented Jul 6, 2021

View reviewed changes

github/repos_hooks_deliveries.go Show resolved Hide resolved

Add test for HookDelivery.ParseRequestPayload

f5889f8

gmlewis requested changes Jul 6, 2021

View reviewed changes

mumoshu added 4 commits July 6, 2021 21:46

Restructure populatePageValues to reduce nesting

f6d688a

Use Timestamp instead of time.Time for delivered_at

ff5a47a

Fix HookResponse.Header to Headers for consistency with the underlyin…

dffbb1c

…g json field

Use %v instead of %d for formatting int64

5f0e1e4

Add more tests for ParseRequestPayload

431c77c

Enhance cursor pagination test for more coverage

23922a8

gmlewis requested changes Jul 7, 2021

View reviewed changes

mumoshu added 3 commits July 8, 2021 11:09

Make it clear that cursor and page are mutually exclusive

72c3380

Add periods to field comments

5b86aff

Add comments on HookRequest and HookResponse

194eff6

gmlewis requested changes Jul 8, 2021

View reviewed changes

Accept suggestions from gmlewis with thanks :)

1381c0f

google#1934 (review)

gmlewis approved these changes Jul 9, 2021

View reviewed changes

Add support for listing and getting organization webhook deliveries

0a3739c

Ref google#1933

mumoshu changed the title ~~Add support for listing and getting repository webhook deliveries~~ Add support for listing and getting repository/organization webhook deliveries Jul 12, 2021

gmlewis requested changes Jul 12, 2021

View reviewed changes

github/orgs_hooks_deliveries.go Show resolved Hide resolved

github/orgs_hooks_deliveries.go Show resolved Hide resolved

gmlewis requested a review from wesleimp July 12, 2021 12:57

mumoshu added 2 commits July 12, 2021 23:40

Add copyright notice to orgs_hooks_deliveries.go

fc8d20f

Add blank after if err block for consistency

123bafa

gmlewis approved these changes Jul 13, 2021

View reviewed changes

Parker77 approved these changes Jul 13, 2021

View reviewed changes

gmlewis merged commit 3ad6169 into google:master Jul 13, 2021

mumoshu deleted the webhook-deliveries-api branch July 14, 2021 21:12

gmlewis mentioned this pull request Aug 11, 2021

Bump version of go-github library #2037

Merged

	// ParsePayload parses the event payload. For recognized event types,
	// a value of the corresponding struct type will be returned.
	func (e *Event) ParsePayload() (payload interface{}, err error) {

	DeliveredAt *time.Time `json:"delivered_at"`
	DeliveredAt *Timestamp `json:"delivered_at"`

	Header map[string]string `json:"headers,omitempty"`
	Headers map[string]string `json:"headers,omitempty"`

	u := fmt.Sprintf("repos/%v/%v/hooks/%d/deliveries", owner, repo, id)
	u := fmt.Sprintf("repos/%v/%v/hooks/%v/deliveries", owner, repo, id)

	// Request is populated by GetHookDelivery
	// Request is populated by GetHookDelivery.

	// Response is populated by GetHookDelivery
	// Response is populated by GetHookDelivery.

Add support for listing and getting repository/organization webhook deliveries #1934

Add support for listing and getting repository/organization webhook deliveries #1934

Conversation

mumoshu commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohpintu90 commented Jul 6, 2021

mumoshu commented Jul 6, 2021

mumoshu commented Jul 6, 2021

mumoshu commented Jul 6, 2021

mumoshu commented Jul 6, 2021 • edited Loading

gmlewis commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu commented Jul 8, 2021

mumoshu commented Jul 8, 2021

gmlewis commented Jul 8, 2021

gmlewis commented Jul 8, 2021

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu commented Jul 9, 2021

gmlewis left a comment

Choose a reason for hiding this comment

mumoshu commented Jul 12, 2021

gmlewis commented Jul 12, 2021

mumoshu commented Jul 12, 2021 • edited Loading

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

Parker77 left a comment

Choose a reason for hiding this comment

mumoshu commented Jul 6, 2021 •

edited

Loading

mumoshu Jul 6, 2021 •

edited

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

mumoshu Jul 6, 2021 •

edited

Loading

mumoshu Jul 6, 2021 •

edited

Loading

mumoshu commented Jul 6, 2021 •

edited

Loading

mumoshu commented Jul 12, 2021 •

edited

Loading