Parse Webhook payload into actual event #427

apelisse · 2016-09-07T21:12:25Z

No description provided.

apelisse · 2016-09-07T21:13:01Z

Not sure this works, but would it make sense?

apelisse · 2016-09-09T03:57:11Z

Actually it works

@willnorris is that something that could be merged?

dmitshur · 2016-09-09T06:00:14Z

github/messages.go

+//       // Process event ...
+//     }
+//
+func ParseWebHook(r *http.Request, secretKey []byte) (interface{}, error) {


This returns an interface{}. Can you please help me understand what one would do with the return value of type interface{} from ParseWebHook?

The example just says "// Process event ...", but it's unclear to me how to.

If I look at the implementation of ParseWebHook, then look at what getWebhookEvent does, I'll see that it returns Event.Payload. Then if I refer to documentation for Event.Payload, it begins to make sense:

Payload returns the parsed event payload. For recognized event types, a value of the corresponding struct type will be returned.

The docs of ParseWebHook just say "returns an Event or a WebHookPayload" which isn't as clear IMO.

apelisse · 2016-09-09T17:18:34Z

Updated, does it make more sense @shurcooL ?

willnorris · 2016-09-12T17:38:32Z

@gmlewis could you take a look and compare with our internal webhook processing code. I do like how this reuses the existing logic in Event.Payload.

gmlewis · 2016-09-12T18:38:06Z

@willnorris - It is similar to what we use internally, although the main difference is that we map the eventType to a specific handler, and then pass the payload []byte to the handler, which can then choose to use Event.Payload or not.

gmlewis · 2016-09-12T18:41:52Z

github/messages.go

+// getWebhookEvent parses the payload and request, and build the Event.
+func getWebhookEvent(r *http.Request, payload []byte) (interface{}, error) {
+	eventType := r.Header.Get(eventTypeHeader)
+	if eventType == "push" {


Why does the push eventType need to be handled specially?

apelisse · 2016-09-14T00:03:31Z

@gmlewis Updated

gmlewis · 2016-09-14T00:24:41Z

github/messages_test.go

+	for _, test := range tests {
+		p, err := json.Marshal(test.payload)
+		if err != nil {
+			t.Fatalf("Marshal(%#v): %v", test.payload, err)


For all of these test outputs, you might prefer "%+v" over "%#v" since we are dealing with pointers to structs... but your call... see which you output you think is clearer.

Did you see this comment @apelisse?

It's not mandatory to change this, I just want to find out if you saw it and decided not to change it, or didn't see the comment.

@apelisse, did you see this?

Yes, I did, I answered (but never published my comment). I like the %#v more because it prints the type, and we care about the type a lot here

gmlewis · 2016-09-14T00:26:40Z

A few minor nits - but I like how this turned out.
LGTM after the minor tweaks.

dmitshur · 2016-09-14T01:38:03Z

github/messages.go

 }
+
+// getWebhookEvent parses the payload and request, and build the Event.
+func getWebhookEvent(messageType string, payload []byte) (interface{}, error) {


Rename to parseWebhookEvent, that seems like a much more fitting and consistent prefix than "get".

Also, should it be "Webhook" or "WebHook"? I see both used in this PR. But the rest of the codebase is also inconsistent. The exported code seems to use "WebHook", so we should probably go with that (since it's harder to change compared to documentation).

gmlewis · 2016-09-14T17:14:38Z

Awaiting LGTM from @shurcooL

apelisse · 2016-09-14T17:15:28Z

Yes, please wait, I'm about to push a new version :-)

apelisse · 2016-09-14T17:44:46Z

@shurcooL updated

A couple of points:

Not sure where you saw WebHook being used rather than Webhook. The existing documentation reads Webhook
I added a ParseWebhookType function. I don't want ParseWebhook() to depend on the request

Hope you like it!

dmitshur · 2016-09-14T19:37:37Z

Thanks, I'll be able to review this tonight.

Not sure where you saw WebHook being used rather than Webhook. The existing documentation reads Webhook

I see WebHook used in all the exported types.

See https://godoc.org/github.com/google/go-github/github.

apelisse · 2016-09-14T20:30:30Z

OK Good catch, let me change that now so you can review it tonight.

Thanks

apelisse · 2016-09-14T20:33:31Z

Updated

dmitshur

There are many comments, test names, error messages that are out of date, please fix those. I've left comments inline.

Many of your sentences are missing periods. It's better to include them to be consistent with the rest of the code and the Go style.

Finally, I've made a suggestion to rename ParseWebHookType func.

I don't see any other issues. Thanks!

dmitshur · 2016-09-15T07:14:57Z

github/messages_test.go

+		if err != nil {
+			t.Fatalf("getWebHookEvent: %v", err)
+		}
+		if !reflect.DeepEqual(test.payload, got) {


The common style is to compare got with want, in that order. Not the other way around.

github $ cat *.go | grep "got, want" | wc -l 152 github $ cat *.go | grep "want, got" | wc -l 0

It might be more readable as:

if want := test.payload; !reflect.DeepEqual(got, want) {

dmitshur · 2016-09-15T07:15:32Z

github/messages_test.go

+			t.Fatalf("getWebHookEvent: %v", err)
+		}
+		if !reflect.DeepEqual(test.payload, got) {
+			t.Errorf("getWebHook(%#v, %#v) = %#v, want %#v", test.messageType, p, got, test.payload)


getWebHook is out of date, it's called ParseWebHook now.

dmitshur · 2016-09-15T07:15:57Z

github/messages_test.go

 	}
 }
+
+func TestGetWebHookEvent(t *testing.T) {


It should be TestParseWebHook now.

dmitshur · 2016-09-15T07:23:50Z

github/messages.go

+}
+
+// ParseWebHook parses the request into an interface to an activity
+// event type (as returned by Event.Payload())


This sentence is missing a period at the end, which is inconsistent.

The phrasing could be improved. There's no more request, its parameter is payload []byte. No need to mention interface, that's already in the return signature. What it returns is a struct type corresponding to the event type. Taking from Event.Payload docs, perhaps something like this:

// ParseWebHook parses the event payload. For recognized event types, a value of // the corresponding struct type will be returned (as returned by Event.Payload()).

dmitshur · 2016-09-15T07:26:34Z

github/messages_test.go

+		}
+		got, err := ParseWebHook(test.messageType, p)
+		if err != nil {
+			t.Fatalf("getWebHookEvent: %v", err)


Same here. "ParseWebHook".

dmitshur · 2016-09-15T07:28:24Z

github/messages.go

 }
+
+// ParseWebHookType returns the type of payload
+func ParseWebHookType(r *http.Request) string {


In this case, I wouldn't call this func parse.

Parse is a good name for something that can fail, and the signature usually has (Something, error) form.

Here, you're really just getting the type of payload from the request headers.

How about renaming it to just WebHookType(r *http.Request)? It feels like leaving out Get is better (see https://golang.org/doc/effective_go.html#Getters).

// WebHookType returns the event type of webhook request r. func WebHookType(r *http.Request) string {

If not that, then call it GetWebHookType, but ParseWebHookType is definitely not the best name.

dmitshur · 2016-09-15T07:29:04Z

github/messages.go

 	sha512Prefix = "sha512"
 	// signatureHeader is the GitHub header key used to pass the HMAC hexdigest.
 	signatureHeader = "X-Hub-Signature"
+	// eventTypeHeader is the Github header key used to pass the event type


Minor style nitpick. This sentence is missing a period at the end, which is inconsistent with the Go style and this codebase.

dmitshur · 2016-09-15T07:30:01Z

github/messages.go

+)
+
+var (
+	// eventTypeMapping is a conversion map between go-github types and webhooks types


Also missing a period.

The phrasing is odd. Isn't it a map from webhook types (e.g., "commit_comment") and go-github types (e.g., CommitCommentEvent)? The current phrasing makes it sound like the inverse of that.

dmitshur · 2016-09-15T07:30:23Z

github/messages.go

+// ParseWebHook parses the request into an interface to an activity
+// event type (as returned by Event.Payload())
+//
+// secretKey is the GitHub Webhook secret message.


There's no secret key anymore.

dmitshur · 2016-09-15T07:30:54Z

github/messages.go

+//       }
+//     }
+//
+func ParseWebHook(messageType string, payload []byte) (interface{}, error) {


This signature looks as nice as I expect it'll ever look, no further suggestions from me.

apelisse · 2016-09-15T20:59:59Z

Updated, I think I've addressed each individual comment. There are no sentence not ending with dots.

Thanks again for your excellent feedback!

dmitshur

Thanks again for your excellent feedback!

No problem!

Updated, I think I've addressed each individual comment. There are no sentence not ending with dots.

Thanks! But it looks like you missed exactly 2 comments, didn't you? Everything else looks great.

dmitshur · 2016-09-16T09:14:02Z

github/messages.go

+)
+
+var (
+	// eventTypeMapping is a conversion map between go-github types and webhooks types.


What about improving the phrasing to be more "left-to-right", e.g., it can say:

// eventTypeMapping maps webhooks types to their corresponding go-github struct types.

Or is that not a more accurate statement?

OK Let's use that exact sentence, it's perfect (I think)

apelisse · 2016-09-16T16:17:20Z

Updated :-)

dmitshur · 2016-09-17T04:54:54Z

You've addressed all my feedback. Thanks for your patience working through these!

I'll let @gmlewis or @willnorris take it from here.

gmlewis · 2016-09-20T05:01:52Z

Very nice! Thank you, @apelisse and @shurcooL !
LGTM. Merging.

gmlewis · 2016-09-20T05:15:08Z

Integrated in 94a3cd9

Closes google#427. Change-Id: I67faac8df63e0d55fcce4ca5f9ab50e26c04d6b3

dmitshur reviewed Sep 9, 2016
View reviewed changes

apelisse force-pushed the add-parse-event branch from 9528f84 to 01a4527 Compare September 9, 2016 17:18

gmlewis assigned gmlewis and unassigned gmlewis Sep 12, 2016

gmlewis reviewed Sep 12, 2016
View reviewed changes

apelisse force-pushed the add-parse-event branch 2 times, most recently from f5c3676 to 3cb4211 Compare September 13, 2016 23:59

gmlewis reviewed Sep 14, 2016
View reviewed changes

gmlewis added the lgtm label Sep 14, 2016

dmitshur reviewed Sep 14, 2016
View reviewed changes

gmlewis removed the lgtm label Sep 14, 2016

apelisse force-pushed the add-parse-event branch from 3cb4211 to 653e0a0 Compare September 14, 2016 17:42

apelisse force-pushed the add-parse-event branch from 653e0a0 to a577815 Compare September 14, 2016 20:33

dmitshur requested changes Sep 15, 2016

View reviewed changes

apelisse force-pushed the add-parse-event branch from a577815 to 7c16ba8 Compare September 15, 2016 20:53

dmitshur reviewed Sep 16, 2016

View reviewed changes

Parse Webhook payload into actual event

3a3d51b

apelisse force-pushed the add-parse-event branch from 7c16ba8 to 3a3d51b Compare September 16, 2016 15:53

dmitshur approved these changes Sep 17, 2016

View reviewed changes

gmlewis closed this in 94a3cd9 Sep 20, 2016

apelisse mentioned this pull request Nov 5, 2016

Remove boilerplate for receiving webhooks #466

Closed

gmlewis mentioned this pull request Apr 18, 2017

Function to get the Delivery ID for a webhook request #618

Closed

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Parse Webhook payload into actual event

c6ff447

Closes google#427. Change-Id: I67faac8df63e0d55fcce4ca5f9ab50e26c04d6b3

Parse Webhook payload into actual event #427

Parse Webhook payload into actual event #427

Uh oh!

Conversation

apelisse commented Sep 7, 2016

Uh oh!

apelisse commented Sep 7, 2016

Uh oh!

apelisse commented Sep 9, 2016

Uh oh!

dmitshur Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willnorris commented Sep 12, 2016

Uh oh!

gmlewis commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Sep 14, 2016

Uh oh!

apelisse commented Sep 14, 2016

Uh oh!

apelisse commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Sep 14, 2016

Uh oh!

apelisse commented Sep 14, 2016

Uh oh!

apelisse commented Sep 14, 2016

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmitshur Sep 9, 2016 •

edited

Loading

dmitshur Sep 9, 2016 •

edited

Loading

apelisse commented Sep 9, 2016 •

edited

Loading

apelisse commented Sep 14, 2016 •

edited

Loading

dmitshur Sep 15, 2016 •

edited

Loading

dmitshur Sep 15, 2016 •

edited

Loading

dmitshur Sep 15, 2016 •

edited

Loading

dmitshur Sep 15, 2016 •

edited

Loading

dmitshur Sep 16, 2016 •

edited

Loading

dmitshur commented Sep 17, 2016 •

edited

Loading