Convert github source to cloud events. by n3wscott · Pull Request #340 · knative/eventing

n3wscott · 2018-08-10T17:27:51Z

This is a followup to #276. The focus is to migrate the Source and Sample to use the cloudevent naming style and the eventing framework.

Fixes #334

Fixes #332

evankanderson · 2018-08-10T17:51:21Z

+
+- dev.knative.github.pullrequest
+
+Uses two images, the feedlet and the receive adapter.


Does the github source require any parameters to be set? If so, what do they mean?

evankanderson · 2018-08-10T17:53:28Z

 	}
 	switch eventType {
-	case "pullrequest":
+	case pullrequestEvent:


This feels like it should be a map[string]string, or a map[string][]string, rather than a switch.

Using a map would also make it easier to report the known event types on line 462.

evankanderson · 2018-08-10T17:58:27Z

+	source := pl.PullRequest.HTMLURL
+
+	var eventType string
+	if len(header["X-GitHub-Event"]) == 1 {


Try:

// global ceTypes = map[string]string{"pull_request": github.PullrequestEvent, "": "unknown"} ... eventType := ceTypes[header.Get("x-github-event")]

evankanderson · 2018-08-10T17:58:43Z

+
+	}
+
+	eventID := "unknown"


Again, prefer Header.Get

evankanderson · 2018-08-10T18:01:19Z

+
+	}
+
+	eventID := "unknown"


Also, should this be a UUID or something if not supplied, rather than a deterministic value?

evankanderson · 2018-08-10T18:04:09Z

-		return err
-	}
-
+func postMessage(target string, m *gh.PullRequestPayload, source, eventType, eventID string) error {


If you leave m as an interface{}, can you avoid the cast in 59 (which could panic), and support multiple payload types idiomatically? (This may not work, I haven't tested.)

this is good.

evankanderson · 2018-08-10T18:05:43Z

 	}
 	defer resp.Body.Close()
-	log.Printf("Error: response Status: %s", resp.Status)
+	log.Printf("response Status: %s", resp.Status)


Do we want to/should we log these?

I moved it to only log on error.

n3wscott · 2018-08-10T20:38:19Z

/retest

n3wscott · 2018-08-10T20:57:23Z

/retest

srinivashegde86 · 2018-08-10T21:38:48Z

/retest

evankanderson · 2018-08-10T22:00:12Z

+
+```
+
+The Feedlet requires a ServiceAccount to run as a cluster admin for the targeted namespace: 


It's unfortunate that we can't put this in a yaml file somewhere to happen at the same time as the controller is installed/the source is created.

evankanderson · 2018-08-10T22:00:41Z

+	"":             UnsupportedEvent,
+}
+
+var GithubEventType = map[string]string{


Is this just the reverse of CloudEventType?

yes, is there a better way?

Build it automatically, like:

var GithubEventType map[string]string for k, v := range(CloudEventType) { GitHubEventType[v] = k }

evankanderson · 2018-08-10T22:02:39Z

 kind: EventType
 metadata:
-  name: pullrequest
+  name: dev.knative.github.pullrequest


Self-registration would make it easier to make this name align with the one in events.go

evankanderson · 2018-08-10T22:04:51Z

-	// TODO: Add more supported event types.
-	default:
+	event, ok := github.GithubEventType[eventType]
+	if !ok {


You may also be able to check if event == ""

evankanderson · 2018-08-10T22:07:20Z

-	pl := payload.(github.PullRequestPayload)
-	postMessage(h.target, &pl)
+
+	hdr := http.Header(header)


I think a webhooks.Header is an http.Header, so it should already have .Get().

I do not have access to .Get() from the webhooks.Header object for some reason, it was a compile error when I was trying.

Aha -- http.Header is an http.Header, which is a map[string[]string. But since it has a different type name, methods which take an http.Header as a receiver don't apply to webhooks.Header.

So glad they "minimize imports": https://github.com/go-playground/webhooks/blob/v3.13.0/webhooks.go#L8

It looks like this declaration was removed in v5.

evankanderson · 2018-08-10T22:17:56Z

+
+	source := pl.PullRequest.HTMLURL
+
+	eventType, ok := github.CloudEventType[hdr.Get("X-GitHub-Event")]


github.CloudEventType should map "" to UnsupportedEvent

It does, this catches all other github events that have a real name but are not "" and "pull_request"

Ah, right. I was thinking about the "header missing" case.

evankanderson · 2018-08-10T22:19:31Z

+		// TODO: in general, receive adapters may have to be able to retry for error cases.
+		log.Printf("response Status: %s", resp.Status)
+		body, _ := ioutil.ReadAll(resp.Body)
+		log.Printf("response Body: %s", string(body))


If this is a 500, should we return an error?

/idk

There are some questions here that are bigger than this PR around what the Receive Adapter should do if it gets an error from the "action". Should it try again? How many times?...

n3wscott · 2018-08-11T19:30:28Z

Grant for LGTM:
/cc @grantr

Evan for Approval:
/assign @evankanderson

evankanderson

/lgtm
/approve

evankanderson · 2018-08-13T20:27:15Z

+	"":             UnsupportedEvent,
+}
+
+var GithubEventType = map[string]string{


Build it automatically, like:

var GithubEventType map[string]string for k, v := range(CloudEventType) { GitHubEventType[v] = k }

evankanderson · 2018-08-13T20:36:32Z

-	pl := payload.(github.PullRequestPayload)
-	postMessage(h.target, &pl)
+
+	hdr := http.Header(header)


Aha -- http.Header is an http.Header, which is a map[string[]string. But since it has a different type name, methods which take an http.Header as a receiver don't apply to webhooks.Header.

So glad they "minimize imports": https://github.com/go-playground/webhooks/blob/v3.13.0/webhooks.go#L8

It looks like this declaration was removed in v5.

evankanderson · 2018-08-13T20:39:38Z

+
+	source := pl.PullRequest.HTMLURL
+
+	eventType, ok := github.CloudEventType[hdr.Get("X-GitHub-Event")]


Ah, right. I was thinking about the "header missing" case.

knative-prow-robot · 2018-08-13T20:41:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Convert github source to cloud events. * Add more words to the readme. * Use cloud event map and updat the readme.

* Added hostnames to tls certificates * Update broker-filter-tls-certificate.yaml --------- Signed-off-by: Calum Murray <cmurray@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Convert github source to cloud events.

1663eeb

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 10, 2018

knative-prow-robot requested review from evankanderson and inlined August 10, 2018 17:28

Add more words to the readme.

bb5822c

evankanderson reviewed Aug 10, 2018

View reviewed changes

Use cloud event map and updat the readme.

54767b0

evankanderson reviewed Aug 10, 2018

View reviewed changes

knative-prow-robot assigned evankanderson Aug 11, 2018

knative-prow-robot requested a review from grantr August 11, 2018 19:30

n3wscott mentioned this pull request Aug 13, 2018

Rewriting the GitHub Event Sample README #343

Closed

evankanderson reviewed Aug 13, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2018

knative-prow-robot merged commit 8536f0e into knative:master Aug 13, 2018

n3wscott added a commit to n3wscott/eventing that referenced this pull request Aug 14, 2018

Convert github source to cloud events. (knative#340)

3418c07

* Convert github source to cloud events. * Add more words to the readme. * Use cloud event map and updat the readme.

tanzeeb mentioned this pull request Aug 24, 2018

Rename pullrequest to dev.knative.github.pullrequest in Github sample #395

Closed

matzew added a commit to matzew/eventing that referenced this pull request Nov 20, 2019

changes towards 0.10 / srvless 1.2.0 (knative#340)

96480f9


		- dev.knative.github.pullrequest

		Uses two images, the feedlet and the receive adapter.


		```

		The Feedlet requires a ServiceAccount to run as a cluster admin for the targeted namespace:


		source := pl.PullRequest.HTMLURL

		eventType, ok := github.CloudEventType[hdr.Get("X-GitHub-Event")]

Conversation

n3wscott commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

n3wscott commented Aug 10, 2018

Uh oh!

n3wscott commented Aug 10, 2018

Uh oh!

srinivashegde86 commented Aug 10, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Aug 11, 2018

Uh oh!

evankanderson left a comment

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!

knative-prow-robot commented Aug 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

n3wscott commented Aug 10, 2018 •

edited

Loading