Skip to content

Add a minimal-dependencies working example listening to k8s events#240

Merged
google-prow-robot merged 13 commits into
knative:masterfrom
evankanderson:1-10th-event
Jul 24, 2018
Merged

Add a minimal-dependencies working example listening to k8s events#240
google-prow-robot merged 13 commits into
knative:masterfrom
evankanderson:1-10th-event

Conversation

@evankanderson
Copy link
Copy Markdown
Member

Fixes #203

Proposed Changes

  • Adds a sample which uses Feed and Service to listen to k8s events.
  • Documents using some of the released eventing components (still TODO on how to document the release, probably in eventing/README.md).
  • Uses dep to vendor the dependencies of function.go for the moment. I'll send a PR to remove the glog usage from https://github.com/knative/eventing/tree/master/pkg/event later today, which may remove the need to use dep to avoid double declaration of the log_dir flag.
  • Ran through the sample twice, but may have missed bugs or assumptions.

@googlebot
Copy link
Copy Markdown

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 the commit author(s) and merge this pull request when appropriate.

@google-prow-robot google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved labels Jul 24, 2018
@evankanderson
Copy link
Copy Markdown
Member Author

@evankanderson
Copy link
Copy Markdown
Member Author

Let me see if I can excise the other commits that somehow leaked into my repo.

@google-prow-robot google-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 24, 2018
Copy link
Copy Markdown
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/hold

This is really good. @evankanderson I made some optional suggestions; feel free to cancel the hold to get this in now.

FYI I didn't actually go through the steps, just read the instructions and yaml files.

Comment thread eventing/samples/k8s-events/README.md Outdated
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:
```shell
kubectl apply -f https://storage.googleapis.com/knative-releases/latest/release-eventing.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This url is now https://storage.googleapis.com/knative-releases/eventing/latest/release.yaml

Comment thread eventing/samples/k8s-events/README.md Outdated
`k8sevents` EventSource.

```shell
kubectl apply -f https://storage.googleapis.com/knative-releases/latest/release-eventing-clusterbus-stub.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This url is now https://storage.googleapis.com/knative-releases/eventing/latest/release-clusterbus-stub.yaml

Comment thread eventing/samples/k8s-events/README.md Outdated

```shell
kubectl apply -f https://storage.googleapis.com/knative-releases/latest/release-eventing-clusterbus-stub.yaml
kubectl apply -f https://storage.googleapis.com/knative-releases/latest/release-eventing-source-k8sevents.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This url is now https://storage.googleapis.com/knative-releases/eventing/latest/release-source-k8sevents.yaml

Comment thread eventing/samples/k8s-events/README.md Outdated
## Build and deploy the sample

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add backticks around {username}

Comment thread eventing/samples/k8s-events/README.md Outdated

1. After the build has completed and the container is pushed to Docker Hub, you
can deploy the function into your cluster. **Ensure that the container image
value in function.yaml matches the container you built in the previous step.**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add backticks around function.yaml. Maybe add an example of the image value?

Comment thread eventing/samples/k8s-events/README.md Outdated
kubectl apply -f eventing/samples/k8s-events/flow.yaml
```

1. You can read the function logs using Kibana. To access Kibana, you need to
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth a footnote mentioning that Kibana isn't installed in the lite or no-mon installs and thatkubectl logs is an alternative.

revisionTemplate:
spec:
container:
# Replace this image with your {username}/k8s-events container
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a TODO or some other callout to be clear that this requires action from the user.

Comment thread eventing/samples/k8s-events/flow.yaml Outdated
# Future revisions may opt to have a meaningful resource string
# that allows users to scope which objects they choose based on
# a templatized self-link. E.g. Configurations across all namespaces:
# /apis/serving.knative.dev/v1alpha1/namespaces/{namespace}/configurations/{config}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably remove all of these comment lines. They're distracting and won't have meaning to the new user.

@google-prow-robot google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2018
@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@evankanderson
Copy link
Copy Markdown
Member Author

updated, thanks

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Jul 24, 2018

/lgtm
/hold cancel

@google-prow-robot google-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 24, 2018
@google-prow-robot google-prow-robot merged commit 32f3f94 into knative:master Jul 24, 2018
@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jul 24, 2018

/lgtm
/approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants