Adding GitHub Sample for move from eventing. by n3wscott · Pull Request #339 · knative/docs

n3wscott · 2018-08-20T14:59:01Z

Moving GitHub eventing sample from knative/eventing to knative/docs.

n3wscott · 2018-08-20T20:22:00Z

/retest

Grant for LGTM:
/cc @grantr

Evan for Approval:
/assign @evankanderson

n3wscott · 2018-08-20T22:13:12Z

related to knative/eventing#343

n3wscott · 2018-08-20T22:14:14Z

/test pull-knative-docs-integration-tests

n3wscott · 2018-08-20T22:18:54Z

READY! PTAL.

grantr · 2018-08-20T22:24:19Z

Is this intended to be exactly the same content as knative/eventing#343?

n3wscott · 2018-08-20T22:37:05Z

Is this intended to be exactly the same content as knative/eventing#343?

Yes, but this branch is now slightly ahead in the samples code.

n3wscott · 2018-08-20T22:56:28Z

istio...

/test pull-knative-docs-integration-tests

samodell · 2018-08-21T18:36:49Z

Taking a look from the docs perspective today

samodell · 2018-08-21T18:40:08Z

+`(looks pretty legit)` to the PR title.
+
+## Prerequisites
+


Below the "Prerequisites" heading, add: "You will need:"

samodell · 2018-08-21T19:27:06Z

+  if you need to create one.
+- [Docker](https://www.docker.com/) installed and running on your local machine,
+  and a Docker Hub account configured (you'll use it for a container registry).
+- The Kubernetes cluster also has Knative eventing core installed. You can


Suggestion: "The Kubernetes cluster also has Knative eventing core installed." --> "Knative eventing core installed on your Kubernetes cluster."

samodell · 2018-08-21T19:27:35Z

+## Configuring Knative
+
+To use this sample, you'll need to install the `stub` ClusterBus and the 
+`github` EventSource.


"EventSource." --> "EventSource:"

samodell · 2018-08-21T19:30:04Z

+`github` EventSource.
+
+```shell
+kubectl apply -f https://storage.googleapis.com/knative-releases/eventing/latest/release-clusterbus-stub.yaml


Might add comments here; IE:
# Installs ClusterBus
# Installs EventSource

(This parallels the k8sevents doc.)

samodell · 2018-08-21T19:31:02Z

@@ -0,0 +1,162 @@
+# GitHub Flow


"Github Flow" --> "Creating a GitHub Flow Using Knative Eventing", or similar.

How about "Reacting to GitHub Events"

samodell · 2018-08-21T19:31:48Z

+kubectl apply -f eventing/samples/github-events/auth.yaml
+```
+
+## Build and deploy the sample


"Build and deploy the sample" --> "Building and deploying the sample"

k8sevents has the same issue.

Do we have a standard template for these somewhere?

samodell · 2018-08-21T19:33:26Z

+
+## Understanding what happened
+
+<!--TODO:


Can this be fleshed out now? I feel like there's a good start in the introduction, and it would add a lot to the sample.

+1 -- much of this should be able to be copied from the k8sevents source, with some additional detail on how the Feed for github has a provisioning step which both calls the GitHub API and creates an underlying service (receive adapter). The rest of the text should be able to be shared with the k8sevent doc.

Requesting this update be a follow-up PR

I don't love leaving a blank section in the guide, though. If you're set on not adding anything here for the moment, I'd move the heading inside the TODO so the rendered version of the file doesn't look like it has a blank section. @n3wscott

samodell · 2018-08-21T19:34:49Z

+
+1.  Use Docker to build the sample code into a container. To build and push with
+    Docker Hub, run these commands replacing `{username}` with your Docker Hub
+    username. Run the following from the _root_ of the `knative/docs` repo:


"run these commands replacing {username} with your Docker Hub username" -->
"run the following commands, replacing {username} with your Docker Hub username"

samodell · 2018-08-21T19:36:23Z

+  [assign a static IP address](https://github.com/knative/docs/blob/master/serving/gke-assigning-static-ip-address.md)
+  instructions.
+
+## Configuring Knative


"Configuring Knative" doesn't really seem to describe what's going on here. Too generic.

Maybe "Installing ClusterBus and EventSource"? "Installing Eventing dependencies"? "Configuring Knative for Eventing"?

FWIW, this matches the k8sevents sample, so we should probably update both at the same time.

ok it will be a followup.

evankanderson

I'm reviewing this in favor of knative/eventing#343, and will close that when this is submitted.

evankanderson · 2018-08-22T17:29:20Z

+FROM golang AS builder
+
+WORKDIR /go/src/github.com/knative/docs/
+ADD . /go/src/github.com/knative/docs/


It looks like you don't need to use dep, so this is much cleaner than what I had to do in the k8sevents commit (see f00e99d and d1b685a for the dep changes). Prior to needing dep, I used the following, which is even simpler than what you have here, and works from the sample directory:

FROM golang AS builder ADD . /go/src/k8s-events # Build the event processing function command inside the container. # (You may fetch or manage dependencies here, # either manually or with a tool like "godep".) RUN go get k8s.io/api/core/v1 github.com/knative/eventing/pkg/event RUN go install k8s-events FROM gcr.io/distroless/base COPY --from=builder /go/src/github.com/knative/docs/k8s-events /sample ...

I copied the k8s event docker file and changed the path of the runable. I was able to get this to work testing the README script just fine.

evankanderson · 2018-08-22T17:31:46Z

+`Channel`, and the `Subscription`.
+
+In response to a pull request event, the sample app _legit_ Service will add 
+`(looks pretty legit)` to the PR title.


I'd put this first -- this is actually what the interesting part of the sample is.

evankanderson · 2018-08-22T17:32:11Z

+- A domain name that allows GitHub to call into the cluster: Follow the
+  [configure a custom domain](https://github.com/knative/docs/blob/master/serving/using-a-custom-domain.md) and 
+  [assign a static IP address](https://github.com/knative/docs/blob/master/serving/gke-assigning-static-ip-address.md)
+  instructions.


Is this the order the instructions should be followed in (custom domain first, then static IP)?

It makes more sense to get a static ip and then use that for the domain... flipped them.

evankanderson · 2018-08-22T17:32:40Z

+  ```
+- A domain name that allows GitHub to call into the cluster: Follow the
+  [configure a custom domain](https://github.com/knative/docs/blob/master/serving/using-a-custom-domain.md) and 
+  [assign a static IP address](https://github.com/knative/docs/blob/master/serving/gke-assigning-static-ip-address.md)


@samodell Should these be relative links?

evankanderson · 2018-08-22T17:38:33Z

+  [assign a static IP address](https://github.com/knative/docs/blob/master/serving/gke-assigning-static-ip-address.md)
+  instructions.
+
+## Configuring Knative


FWIW, this matches the k8sevents sample, so we should probably update both at the same time.

evankanderson · 2018-08-22T17:50:10Z

+
+## Cleaning up
+
+To clean up the function, `Flow`, auth, and secret:


Do we need to clean up auth.yaml? I think it's needed to make the github source able to operate at all.

Yes but I need help because I have not looked into what rbac arguments should look like

evankanderson · 2018-08-22T17:53:58Z

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


Can we avoid making this cluster-admin? Something like the following definition (instead of ClusterRoleBinding) should be sufficient IIUC:

--- apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: name: create-knative-service namespace: default rules: - apiGroups: ["serving.knative.dev"] resources: ["services"] verbs: ["get", "list", "watch", "create", "update", "delete", "patch"] --- # This enables the feed-sa to deploy the receive adapter. apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding metadata: name: feed-sa-deploy namespace: default subjects: - kind: ServiceAccount name: feed-sa namespace: default roleRef: kind: Role name: create-knative-service apiGroup: rbac.authorization.k8s.io

awesome. 👍 thanks!!

evankanderson · 2018-08-22T17:55:23Z

+  serviceAccountName: feed-sa
+  trigger:
+    eventType: dev.knative.github.pullrequest
+    resource: n3wscott/kopond


You probably want to update 5 to have the user change this resource before creating the Flow.

evankanderson · 2018-08-22T17:55:37Z

+          key: githubCredentials
+  action:
+    target:
+      kind: Route


evankanderson · 2018-08-22T17:58:05Z

+
+	// Set up the auth for being able to talk to Github. It's
+	// odd that you have to also pass context around for the
+	// calls even after giving it to client. But, whatever.


Is this a rant about Go?

It is Ville's rant. hahahah

n3wscott · 2018-08-22T22:28:26Z

/test pull-knative-docs-integration-tests

n3wscott · 2018-08-27T14:24:23Z

PTAL?

samodell · 2018-08-27T22:58:36Z

/lgtm
Will approve after blank section is either fleshed out or the heading is moved within the TODO comment.

samodell · 2018-08-27T23:10:06Z

/lgtm
/approve

knative-prow-robot · 2018-08-27T23:10:12Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [samodell]

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

Staging for move from eventing.

5b4477d

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2018

knative-prow-robot requested review from evankanderson and rgregg August 20, 2018 14:59

n3wscott changed the title ~~[WIP] Staging for move from eventing.~~ [WIP] Staging GitHub Sample for move from eventing. Aug 20, 2018

n3wscott mentioned this pull request Aug 20, 2018

Rewriting the GitHub Event Sample README knative/eventing#343

Closed

evankanderson mentioned this pull request Aug 20, 2018

Delete GitHub samples from eventing. knative/eventing#383

Merged

knative-prow-robot assigned evankanderson Aug 20, 2018

knative-prow-robot requested a review from grantr August 20, 2018 20:22

the event name needed to be updated.

14be1b2

n3wscott changed the title ~~[WIP] Staging GitHub Sample for move from eventing.~~ Adding GitHub Sample for move from eventing. Aug 20, 2018

knative-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 Aug 20, 2018

Dockerfile worked just fine.

d9eda3b

Updated based on feedback.

b8c3e8e

core core core.

4820035

samodell self-assigned this Aug 21, 2018

samodell suggested changes Aug 21, 2018

View reviewed changes

evankanderson reviewed Aug 22, 2018

View reviewed changes

Updated based on feedback.

6e0ab61

samodell approved these changes Aug 27, 2018

View reviewed changes

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

Add todo to add to Understanding what happened.

954f997

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

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

knative-prow-robot added the approved label Aug 27, 2018

knative-prow-robot merged commit b37d9ab into knative:master Aug 27, 2018


		## Cleaning up

		To clean up the function, `Flow`, auth, and secret:

Conversation

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

grantr commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

samodell commented Aug 21, 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!

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!

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!

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!