GitHub Receive adapter as a serving Service. by n3wscott · Pull Request #276 · knative/eventing

n3wscott · 2018-07-24T17:52:31Z

Fixes #208

vaikas · 2018-07-25T05:01:14Z

 	serviceName, ok := statusMap[targetFieldName]
 	if !ok {
-		return "", fmt.Errorf("%q does not contain field %q in status", targetFieldName, ref.Name)
+		return "", fmt.Errorf("%q/%q does not contain field %q in status", ref.Kind, ref.Name, targetFieldName)


you probably don't want %q/%q here because then both are quoted? Maybe just:
%s/%s

vaikas · 2018-07-25T05:01:25Z

 	serviceNameStr, ok := serviceName.(string)
 	if !ok {
-		return "", fmt.Errorf("%q status field %q is not a string", targetFieldName, ref.Name)
+		return "", fmt.Errorf("%q status field %q/%q is not a string", targetFieldName, ref.Kind, ref.Name)


vaikas · 2018-07-25T05:01:47Z

  source: github
  image: github.com/knative/eventing/pkg/sources/github
+  parameters:
+    image: github.com/knative/eventing/pkg/sources/github/receive_adapter


nit: looks like it's missing a newline here?

vaikas · 2018-07-25T05:08:18Z

+    namespace: default
+roleRef:
+  kind: ClusterRole
+  name: cluster-admin


Since you're making such big changes anyway, can you look at the changes that @evankanderson did for tightening up the rules for k8s events when he moved it under knative/docs/eventing and make it stricter here too.

I took a look at those examples and I ... need to learn more about rbac. I am not sure how to do the same thing for this PR yet. Maybe @evankanderson can help me?

If you know what objects the service account is going to use in Kubernetes, you can basically just make a list.

You can start with create-deployment and feed-sa-deploy from the k8sevents/serviceaccount.yaml. Since you're using a service.serving.knative.dev resource, you'll probably need to add that to the list of rules on the create-deployment Role. (You'll probably also want to pick a different name.)

vaikas · 2018-07-25T05:10:52Z

@@ -15,19 +15,35 @@
 apiVersion: feeds.knative.dev/v1alpha1


we shouldn't need a feed.yaml with flow.yaml anymore.

vaikas · 2018-07-25T05:13:22Z

+            knative.dev/type: function
+        spec:
+          container:
+            image: github.com/knative/eventing/sample/github


Can we make the github function actually then talk back to the github to modify something like it did before?

n3wscott · 2018-07-26T00:11:27Z

@vaikas-google I updated the sample to do what it did before now but it does not run as the github lib wehbook but just a normal webhook.. And then my cluster went craycray today and istio is just throwing 503's for posting to the channel... idk more tomorrow.

n3wscott · 2018-07-26T18:06:09Z

/assign @inlined

inlined

Generally LGTM. As a follow-up, I think we should consider the trouble we have setting up GitHub's webhook to be a pretty common case & simplify it in the core infra. E.g. I think we should copy or factor out what serving does with its single ingress. I also have mixed feelings about the event source being implemented as a Service. On one hand, this lets the system scale down to zero. On the other hand, this means that eventing will never break its dependency on serving. At minimum, I'd want to move this event source out of the core repo.

inlined · 2018-07-26T19:19:57Z

+	postfixReceiveAdapter = "rcvadptr"
+
+	// watchTimeout is the timeout that the feedlet will wait for the Receiver Adapter to get a domain name.
+	watchTimeout int64 = 60 * 5 // 5 minutes?


Nit: time.Duration solves so many bugs by having unambiguous units and forced casts to primitives. I'd strongly prefer this to be

watchTimeout = 5 * time.Minute

and it to be used below as

TimeoutSeconds: int64(watchTimeout / time.Second)

inlined · 2018-07-26T19:20:25Z

+	// watchTimeout is the timeout that the feedlet will wait for the Receiver Adapter to get a domain name.
+	watchTimeout int64 = 60 * 5 // 5 minutes?
+
+	//// secretName is the name of the secret that contains the GitHub credentials.


Why the double comments?

inlined · 2018-07-26T19:29:07Z

+		glog.Error("no Webhook ID Found, bailing...")
+		return nil
+	}
+	webhookID := feedContext.Context[webhookIDKey].(string)


As a matter of practice, I wonder if we should
A) Always , ok our casts. What if this somehow got corrupted? I'm glad you caught the nil case above since that had caused GCP Pub/Sub flows to be undeleteable.
B) Invest in certain framework safeguards (e.g. bail on N panics)

I fixed and added a todo to elevate the helper method to pkg.

inlined · 2018-07-26T19:31:20Z

+	}
+	webhookID := feedContext.Context[webhookIDKey].(string)
+
+	ctx := context.Background()


Not something to fix in this PR, but this again makes me wonder if base libraries should own the root context.Context. Why isn't there a timeout for example?

inlined · 2018-07-26T19:32:30Z

+				return nil
+			}
+		}
+		glog.Errorf("gailed to delete the webhook: %#v", err)


s/gailed/failed

Also, %#v will ignore the error interface format rules. You'll also get pointer addresses instead of actually following anything. https://play.golang.org/p/bNajvEDPpv5

inlined · 2018-07-26T20:40:49Z

+	hook := ghclient.Hook{
+		Name:   &name,
+		URL:    &domain,
+		Events: []string{"pull_request"},


This should be dynamic. We're going to need a map from our CloudEvents types to their native types. Feel free to do a TODO for now I guess or use the trigger.EventType.

inlined · 2018-07-26T20:44:40Z

+	// odd that you have to also pass context around for the
+	// calls even after giving it to client. But, whatever.
+	ctx := context.Background()
+	//ts := oauth2.StaticTokenSource(


Please yank commented out code

inlined · 2018-07-26T20:48:43Z

+	hook := github.New(&github.Config{Secret: credentials.SecretToken})
+	hook.RegisterEvents(h.HandlePullRequest, github.PullRequestEvent)
+
+	err = webhooks.Run(hook, ":8080", "/")


Should we be using fmt.Sprintf(":%s", os.Getevn("PORT"))?

left a TODO. not sure if it matters because this is not the port that is exposed

inlined · 2018-07-26T20:50:51Z

+    target:
+      kind: Route
+      apiVersion: serving.knative.dev/v1alpha1
+      name: legit


nit: trailing newline

inlined · 2018-07-26T20:51:02Z

      "accessToken": "<YOUR PERSONAL TOKEN FROM GITHUB>",
      "secretToken": "<YOUR RANDOM STRING>"
-    }
+    }


nit: another trailing newline missing

vaikas · 2018-07-27T16:09:50Z

@inlined re: using Service to implement github handler seems totally reasonable to me. It's an implementation detail of a particular receive adapter and not a hard dependency on eventing itself. I'd for example expect these sources to move out of this repo into somewhere else and not be part of the pkg/sources in eventing.

inlined · 2018-07-27T16:20:48Z

@vaikas-google Yeah, I assumed that this would leave the core repo which helps us meet the goal of breaking dependencies. It's hard to not want to use knative/serving; it's just too good a product!

vaikas · 2018-07-27T16:36:05Z

lol, trudat

…

On Fri, Jul 27, 2018 at 9:20 AM Thomas Bouldin ***@***.***> wrote: @vaikas-google <https://github.com/vaikas-google> Yeah, I assumed that this would leave the core repo which helps us meet the goal of breaking dependencies. It's hard to not want to use knative/serving; it's just too good a product! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#276 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKwedJMQ1_kPpHnw8_fCCwA7L1N9DlhPks5uKz3igaJpZM4VdL9v> .

inlined · 2018-08-01T16:59:47Z

FYI I'm headed on vacation tonight. I don't want to leave you stuck with an AWOL reviewer.

/assign @vaikas-google

n3wscott

Thanks for the great review @inlined !! Updated.

n3wscott · 2018-08-01T15:23:44Z

+	// watchTimeout is the timeout that the feedlet will wait for the Receiver Adapter to get a domain name.
+	watchTimeout int64 = 60 * 5 // 5 minutes?
+
+	//// secretName is the name of the secret that contains the GitHub credentials.


n3wscott · 2018-08-01T15:57:44Z

+		glog.Error("no Webhook ID Found, bailing...")
+		return nil
+	}
+	webhookID := feedContext.Context[webhookIDKey].(string)


I fixed and added a todo to elevate the helper method to pkg.

n3wscott · 2018-08-01T16:51:05Z

+		return nil, err
+	}
+
+	glog.Infof("created Service: %+v", service)


TODO added.

n3wscott · 2018-08-01T16:55:41Z

+func receiveAdapterName(resource string) string {
+	serviceName := fmt.Sprintf("%s-%s-%s", "github", resource, postfixReceiveAdapter) // TODO: this needs more UUID on the end of it.
+	serviceName = strings.Replace(serviceName, "/", "-", -1)
+	serviceName = strings.Replace(serviceName, ".", "-", -1)


resources can but there are limitations on knative service I think. I was getting errors for "." because I think it turns the name into a domain. and k8s service has some limitations in addition to k8s resources.

n3wscott · 2018-08-01T16:57:30Z

+		return nil, nil
+	}
+
+	glog.Infof("secretName %s ; secretKey: %s", trigger.Parameters[secretName].(string), trigger.Parameters[secretKey].(string))


That is not the secret value, but the k8s secret resource name and internal key to find the secret. But yes, I need to remove this.

n3wscott · 2018-08-01T17:01:45Z

+	hook := github.New(&github.Config{Secret: credentials.SecretToken})
+	hook.RegisterEvents(h.HandlePullRequest, github.PullRequestEvent)
+
+	err = webhooks.Run(hook, ":8080", "/")


left a TODO. not sure if it matters because this is not the port that is exposed

n3wscott · 2018-08-01T17:05:06Z

/unassign @vaikas-google

Ville is on vacation for a couple weeks.

/assign @mattmoor

n3wscott · 2018-08-02T17:37:17Z

/test pull-knative-eventing-integration-tests

adamharwayne · 2018-08-03T14:44:31Z

+
+The `Flow` will accept the Webhook calls from GitHub and pass them to the _legit_ service.
+
+The `Flow` will configure a Receive Adapter pod from the `


Looks like there should be a link/name at the end of this sentence.

adamharwayne · 2018-08-03T14:45:50Z

 ```

 Then create a PR for the repo you configured the webhook for, and you'll see that the Title
 will be modified with the suffix '(I buy it)'


'(I buy it)' should be replaced with '(looks pretty legit)'.

n3wscott · 2018-08-03T17:48:05Z

/assign @evankanderson

@inlined is on vacation for a bit

n3wscott · 2018-08-03T17:48:22Z

/test pull-knative-eventing-integration-tests

evankanderson

/lgtm
/approve

evankanderson · 2018-08-09T21:15:57Z

+			APIVersion: "v1alpha1",
+		},
+		FieldSelector:  fmt.Sprintf("metadata.name=%s", serviceName),
+		LabelSelector:  "receive-adapter=github",


Do we want to ever "adopt" an existing service if it's for another Flow? It seems like checking an OwnerReference would be the right way to avoid that. It would also make the cleanup easier.

knative-prow-robot · 2018-08-09T22:47:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr, 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

n3wscott · 2018-08-10T01:05:39Z

/retest

mattmoor · 2018-08-10T01:20:38Z

(I'm playing tide since it's gone fishing)

google-prow-robot requested review from inlined and ultrasaurus July 24, 2018 17:52

google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 24, 2018

n3wscott changed the title ~~GitHub Receive adapter as a serving Service. Managed by a feedlet.~~ [WIP] GitHub Receive adapter as a serving Service. Managed by a feedlet. Jul 24, 2018

google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2018

vaikas reviewed Jul 25, 2018

View reviewed changes

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 26, 2018

n3wscott changed the title ~~[WIP] GitHub Receive adapter as a serving Service. Managed by a feedlet.~~ GitHub Receive adapter as a serving Service. Managed by a feedlet. Jul 26, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2018

google-prow-robot assigned inlined Jul 26, 2018

n3wscott changed the title ~~GitHub Receive adapter as a serving Service. Managed by a feedlet.~~ GitHub Receive adapter as a serving Service. Jul 26, 2018

inlined reviewed Jul 26, 2018

View reviewed changes

google-prow-robot assigned vaikas Aug 1, 2018

n3wscott commented Aug 1, 2018

View reviewed changes

google-prow-robot assigned mattmoor and unassigned vaikas Aug 1, 2018

adamharwayne reviewed Aug 3, 2018

View reviewed changes

n3wscott force-pushed the github_channel_service branch from d731ced to aa06033 Compare August 3, 2018 17:39

google-prow-robot assigned evankanderson Aug 3, 2018

Scott Nichols added 18 commits August 9, 2018 10:46

Working exmaple of the legit service.

0395a1c

updating the readme.

260eb4c

Cleanup based on feedback.

15f2bce

Update readme.

32a4598

glog should be gone.

e585c98

update based on feedback.

61afb61

https.

ed724a4

Validate the resource to be owner/repo.

bbe8d08

Clean up error log lines and returned errors.

490d0ab

make pull request event types less hardcoded.

de3ea9c

Updated based on feedback.:

e98e1da

Updated based on feedback.

77dd3cc

Don't panic.

f170410

Validate input.

c5074a7

Need build and serving.

b53ebde

don't add flow again,

0e787d8

remove context, not used.

81f05ef

ran update-deps.

c8a5a6c

n3wscott force-pushed the github_channel_service branch from 8a1a833 to c8a5a6c Compare August 9, 2018 17:56

Adding more todos and comments

3fdf318

evankanderson reviewed Aug 9, 2018

View reviewed changes

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

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

mattmoor merged commit f8a0257 into knative:master Aug 10, 2018

This was referenced Aug 10, 2018

Convert github source to cloud events. #340

Merged

Rewriting the GitHub Event Sample README #343

Closed

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged


		The `Flow` will accept the Webhook calls from GitHub and pass them to the _legit_ service.

		The `Flow` will configure a Receive Adapter pod from the `

Conversation

n3wscott commented Jul 24, 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!

n3wscott commented Jul 26, 2018

Uh oh!

n3wscott commented Jul 26, 2018

Uh oh!

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

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!

vaikas commented Jul 27, 2018

Uh oh!

inlined commented Jul 27, 2018

Uh oh!

vaikas commented Jul 27, 2018 via email

Uh oh!

inlined commented Aug 1, 2018

Uh oh!

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

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 1, 2018

Uh oh!

n3wscott commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment