Rewriting the GitHub Event Sample README by n3wscott · Pull Request #343 · knative/eventing

n3wscott · 2018-08-10T22:25:08Z

Based on feedback from https://github.com/knative/eventing/pull/276/files#diff-f9667ca3c88cf4316ebe5aab2d712207 I have updated the README to be more in-line with what the current docs repo does.

Note: I have also staged the commands for the move to knative/docs/eventing/samples/github-events

@evankanderson : I need guidance on how much of the "What just happened" second needs to be pulled from the k8s event or if we can point to that?

Related to #333

Dependent on #340

n3wscott · 2018-08-10T22:30:41Z

Grant for LGTM:
/assign @grantr

Evan for Approval:
/assign @evankanderson

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

/test pull-knative-eventing-integration-tests

grantr · 2018-08-10T23:50:46Z

/cc @grantr

(cc requests my review, so I can track this in a different email label)

googlebot · 2018-08-14T16:54:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

…ned.

googlebot · 2018-08-14T16:55:25Z

CLAs look good, thanks!

evankanderson

This looks pretty good, thanks for updating!

evankanderson · 2018-08-14T17:54:08Z

+  ```shell
+  kubectl apply -f https://storage.googleapis.com/knative-releases/eventing/latest/release.yaml
+  ```
+- A custom domain is setup to allow for GitHub to be able to call into the cluster,  


evankanderson · 2018-08-14T17:55:02Z

-# This will show the available Subscriptions created from the Flow:
-kubectl get subscriptions -o yaml
+Because the `github` EventSource needs to create a `Service.serving.knative.dev`
+and call the Kubernetes watch API on events, you'll need to provision a special


I don't think github needs to call the Kubernetes watch API, does it?

evankanderson · 2018-08-14T17:55:57Z

-```
-
-Then create the secret so that you can see changes:
+The `eventing/samples/github-events/auth.yaml` file provisions a service account, creates a role


I don't think you want eventing in this path.

In fact, I'd make this a relative path, so that it can be easily moved to knative/docs when ready.

The docs are written to run from the root of the docs directory so I have already changed things to make that assumption: eventing/samples/github-events

evankanderson · 2018-08-14T17:56:12Z

-
-Then create the secret so that you can see changes:
+The `eventing/samples/github-events/auth.yaml` file provisions a service account, creates a role
+which can create Service in the `default` namespace and can view all


"a Knative Service"

evankanderson · 2018-08-14T17:56:27Z

-
-Then create the secret so that you can see changes:
+The `eventing/samples/github-events/auth.yaml` file provisions a service account, creates a role
+which can create Service in the `default` namespace and can view all


Why does it need to view kubernetes resources?

evankanderson · 2018-08-14T18:06:32Z

+    legit             True      <none>
+    ```
+
+1.  Create a [personal access token](https://github.com/settings/tokens) to 


This needs a little more detail. At a minimum:

What scopes should be selected?

Does the name matter?

Mention that you'll need to copy the string once, and if you forget to/lose it, you'll need to generate a new one.

A screenshot would be nice, feel free to leave as a TODO.

Added a screenshot and a paragraph with the requested info.

evankanderson · 2018-08-14T18:07:18Z

+1.  Create a [personal access token](https://github.com/settings/tokens) to 
+    GitHub repo that the GitHub source can use to register webhooks with the 
+    GitHub API. Also decide on a token that your code will use to authenticate
+    the incoming webhooks from GitHub (*accessToken*). Update 


What are the properties that this token should have? Just a unique string?

Maybe there's an easy way to generate a unique 12-character alphanumeric string (pwgen or head -c 8 /dev/urandom | base64encode might work)?

evankanderson · 2018-08-14T18:14:05Z

+    legit             True      <none>
+    ```
+
+1.  Create a [personal access token](https://github.com/settings/tokens) to 


I'd also call out that we use the same token for both the flow (to set up the webhook) and the function (to read and update the PR title).

I'd also move this step up before step 2 (kubectl apply -f function.yaml), so that the secrets are in place and the service has everything it needs before step 3 (wait for service to become ready).

evankanderson · 2018-08-14T18:14:50Z

+
+## Understanding what happened
+
+...TODO...


I'd make this more formal:

TODO: explain the resources and communication channels, as well as where the secret is used.

In particular include a note to look at https://github.com/<owner>/<repo>/settings/hooks to see the webhook registered and then deleted.

evankanderson · 2018-08-14T18:17:52Z

-```
-
-And you can check the repo and see the webhook has been removed
+To clean up the function, `Flow`, auth, and secret:


Also mention deleting the github Personal Access Token from the github UI.

greghaynes · 2018-08-16T18:53:40Z

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


Dockerfile pathing is relative to the dockerfile itself which causes two issues here:

this github-events directory's contents gets placed under /go/src/github.com/knative/docs/ which results in an unable to find eventing/samples/github-events error when compiling. This can be fixed by changing the above command to ADD . /go/src/github.com/knative/docs/eventing/samples/github-events

Because we are only copying the github-events dir we lack the deps for this example (which are in docs/vendor). ISTM we can either add them with go get or add dep to the docker image along with a Gopkg config and then run dep ensure.

Thanks for the tip! I will be sure to fix this up.

…ove.

n3wscott · 2018-08-20T14:30:24Z

Updated, PTAL @evankanderson

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

Follow-up move to docs: knative/docs#339

n3wscott · 2018-08-20T15:08:05Z

Follow-up to delete the github samples from eventing repo: #383

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

I had to use the code from #339

Closes #339

Readme works. Source had two minor issues:

log flag was missing because glog is not used. removed.
istio sidecar injection was preventing the feedlet job from starting. fixed from patch

grantr

/lgtm

Thanks @n3wscott for updating these docs! Just some minor grammar comments.

grantr · 2018-08-20T22:09:16Z

-# This will show the Revision that was created by the Configuration:
-kubectl get revisions -o yaml
-```
+- A Kubernetes cluster with Knative installed. Follow the


This should probably be with Knative serving installed.

grantr · 2018-08-20T22:10:01Z

+  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 core Knative eventing tools installed. You can install them with:


I don't think I'd call these "tools". Maybe The Knative eventing core installed.? Alternately, amend the above to say A Kubernetes cluster with Knative serving and eventing installed.

grantr · 2018-08-20T22:11:35Z

+  ```shell
+  kubectl apply -f https://storage.googleapis.com/knative-releases/eventing/latest/release.yaml
+  ```
+- A custom domain is setup to allow for GitHub to be able to call into the cluster:


The grammar here doesn't line up with the other bullets. I'd rephrase this to A domain name that allows GitHub to call into the cluster.

grantr · 2018-08-20T22:12:45Z

-ko apply -f sample/github/flow.yaml
-```   
+Because the `github` EventSource needs to create a
+`Service.serving.knative.dev`, you'll need to provision a special


I'd recommend using the non-fully scoped name here: EventSource needs to create a Knative Service

grantr · 2018-08-20T22:14:51Z

+    The token can be named anything you find convenient. This sample requires
+    full `repo` control to be able update the title of the _Pull Request_.
+    The Source requires `admin:repo_hook`, this allows it to create webhooks
+    into repos that your account is allowed to do so. Copy and save this token,


Replace the comma with a semicolon: Copy and save this token; GitHub will...

n3wscott · 2018-08-20T22:43:59Z

Updated both knative/docs#339 and here.

n3wscott · 2018-08-20T23:00:59Z

/test pull-knative-eventing-integration-tests

n3wscott · 2018-08-21T00:24:58Z

/test pull-knative-eventing-integration-tests

n3wscott · 2018-08-21T15:18:00Z

/test pull-knative-eventing-integration-tests

knative-prow-robot · 2018-08-21T17:44:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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

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

grantr

/lgtm

grantr · 2018-08-21T17:45:46Z

/assign @evankanderson

knative-prow-robot · 2018-08-21T23:04:25Z

New changes are detected. LGTM label has been removed.

n3wscott · 2018-08-22T21:44:13Z

Closed in favor of knative/docs#339

knative-prow-robot requested review from evankanderson and vaikas August 10, 2018 22:25

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 assigned evankanderson Aug 10, 2018

knative-prow-robot assigned grantr Aug 10, 2018

knative-prow-robot requested a review from grantr August 10, 2018 23:50

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 14, 2018

Updated to the new style of readme. Still need to fill out what happe…

427dd5a

…ned.

n3wscott force-pushed the update-readme-github branch from 4f4487b to 427dd5a Compare August 14, 2018 16:55

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 14, 2018

evankanderson reviewed Aug 14, 2018

View reviewed changes

greghaynes reviewed Aug 16, 2018

View reviewed changes

updating based on feedback and adding a few todos for the docs repo m…

3f7cd73

…ove.

Made the github scopes screenshot more clear.

43b202e

validated instructions, minor fixes to source.

98b278a

n3wscott mentioned this pull request Aug 20, 2018

Adding GitHub Sample for move from eventing. knative/docs#339

Merged

grantr approved these changes Aug 20, 2018

View reviewed changes

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

Syncing PR between docs and here.

7a42e3a

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

core core core.

bfb8c93

grantr reviewed Aug 21, 2018

View reviewed changes

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

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged

Revert changes unrelated.

737a099

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

Revert job.go.

2607b99

n3wscott closed this Aug 22, 2018

Conversation

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

Uh oh!

Uh oh!

n3wscott commented Aug 10, 2018

Uh oh!

n3wscott commented Aug 10, 2018

Uh oh!

grantr commented Aug 10, 2018

Uh oh!

googlebot commented Aug 14, 2018

Uh oh!

googlebot commented Aug 14, 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!

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

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 20, 2018

Uh oh!

n3wscott commented Aug 21, 2018

Uh oh!

n3wscott commented Aug 21, 2018

Uh oh!

n3wscott commented Aug 10, 2018 •

edited

Loading