Added Python Example for Knative Eventing #2299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

knative-prow-robot merged 5 commits into knative:master from axsaucedo:added_knative_eventing_example

Jul 28, 2020

Contributor

axsaucedo commented Mar 11, 2020

Fixes #issue-number or description of the problem the PR solves

Proposed Changes

Added Python Example for Knative Eventing

The example has the exact same contents than the go example.

googlebot added the cla: yes label

Contributor

knative-prow-robot commented Mar 11, 2020

Hi @axsaucedo. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot added needs-ok-to-test size/L labels

mattmoor reviewed

View reviewed changes

knative-prow-robot requested review from lionelvillard and n3wscott

March 11, 2020 15:09


          Added Python Knative Eventing Helloworld Example

68ddf1b

axsaucedo force-pushed the added_knative_eventing_example branch from 42ffb13 to 68ddf1b Compare

March 11, 2020 15:14

mattmoor reviewed

View reviewed changes


          Update docs/eventing/samples/helloworld/helloworld-python/Dockerfile

8de32ce

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

mattmoor reviewed

View reviewed changes


          Update docs/eventing/samples/helloworld/helloworld-python/Dockerfile

ba7ce92

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

mattmoor reviewed

View reviewed changes


          Update docs/eventing/samples/helloworld/helloworld-python/README.md

ea3da45

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

mattmoor reviewed

View reviewed changes


          Apply suggestions from code review

82c085e

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

Contributor Author

axsaucedo commented Mar 11, 2020

Thanks @mattmoor - I've approved all requested changes

carieshmarie added the ok-to-test label

Contributor Author

axsaucedo commented Mar 16, 2020

@mattmoor just wanted to see if there are still any other changes required

Member

mattmoor commented Mar 16, 2020

Ha, sorry those comments were all automated.

/assign @n3wscott

I think @n3wscott is probably a good set of eyeballs for this.

knative-prow-robot assigned n3wscott

Contributor Author

axsaucedo commented Mar 16, 2020

Haha ok thanks for the heads up @mattmoor! @n3wscott let me know if there are any questions to merge

evankanderson reviewed

View reviewed changes

Member

evankanderson left a comment

/assign @abrennan89

If she's still available to take a pass over this as it's a complete new doc, and I don't recall where our best practices have stabilized.

knative-prow-robot assigned abrennan89

Contributor Author

axsaucedo commented Mar 18, 2020

Ok sounds good - @abrennan89 let me know if there are any changes required

abrennan89 suggested changes

View reviewed changes

Contributor

abrennan89 left a comment

Some initial review comments, mostly around tone and structure.

docs/eventing/samples/helloworld/helloworld-python/README.md

		@@ -0,0 +1,338 @@
		A simple web app written in Python that you can use to test knative eventing. It shows how to consume a [CloudEvent](https://cloudevents.io/) in Knative eventing, and optionally how to respond back with another CloudEvent in the http response, by adding the Cloud Eventing headers outlined in the Cloud Events standard definition.

Contributor

abrennan89 Mar 19, 2020

"This guide provides an example of a simple web application, written in Python, that can be used to test Knative Eventing."
We capitalize 'Eventing' throughout the site, so please capitalize all instances. @carieshmarie do we have any official guidelines for Knative about capitalization?

docs/eventing/samples/helloworld/helloworld-python/README.md

		@@ -0,0 +1,338 @@
		A simple web app written in Python that you can use to test knative eventing. It shows how to consume a [CloudEvent](https://cloudevents.io/) in Knative eventing, and optionally how to respond back with another CloudEvent in the http response, by adding the Cloud Eventing headers outlined in the Cloud Events standard definition.

		We will deploy the app as a [Kubernetes Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/) along with a [Kubernetes Service](https://kubernetes.io/docs/concepts/services-networking/service/).

Contributor

abrennan89 Mar 19, 2020

"This example deploys..."
Don't use language like "we will" - see https://developers.google.com/style/tone for guidance on tone.

docs/eventing/samples/helloworld/helloworld-python/README.md

+              A simple web app written in Python that you can use to test knative eventing. It shows how to consume a [CloudEvent](https://cloudevents.io/) in Knative eventing, and optionally how to respond back with another CloudEvent in the http response, by adding the Cloud Eventing headers outlined in the Cloud Events standard definition.
+              We will deploy the app as a [Kubernetes Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/) along with a [Kubernetes Service](https://kubernetes.io/docs/concepts/services-networking/service/).
+              However, you can also deploy the app as a [Knative Serving Service](../../../../serving/README.md).

Contributor

abrennan89 Mar 19, 2020

Remove 'however'

docs/eventing/samples/helloworld/helloworld-python/README.md

		```

		## Before you begin

Contributor

abrennan89 Mar 19, 2020

Add a line to say something like "To follow this example guide, you will need:"

Also, change "we'll use it"
I'd list the Docker Hub a/c as a separate point, e.g.

A configured Docker Hub account to use as a container registry.

docs/eventing/samples/helloworld/helloworld-python/README.md


		```

		1. Add a requirements.txt file containing the following contents:

Contributor

abrennan89 Mar 19, 2020

Where is this txt file added? Also, use backticks for literals? e.g. requirements.txt

Contributor

vmexalien Sep 17, 2020

still need to determine where the requirements.txt file is added

docs/eventing/samples/helloworld/helloworld-python/README.md

+                      kubectl --namespace knative-samples get deployments helloworld-python
+                      kubectl --namespace knative-samples get svc helloworld-python
+                      ```
+. It created a Knative Eventing Trigger to route certain events to the helloworld-python application. Make sure that Ready=true

Contributor

abrennan89 Mar 19, 2020

Again, what is 'it'? Please modify the structure of these steps as mentioned in previous comment.

docs/eventing/samples/helloworld/helloworld-python/README.md

+                      kubectl --namespace knative-samples get trigger helloworld-python
+                      ```
+              ## Send and verify CloudEvents
+              Once you have deployed the application and verified that the namespace, sample application and trigger are ready, let's send a CloudEvent.

Contributor

abrennan89 Mar 19, 2020

Once -> After
Let's -> you can

docs/eventing/samples/helloworld/helloworld-python/README.md

+              Once you have deployed the application and verified that the namespace, sample application and trigger are ready, let's send a CloudEvent.
+              ### Send CloudEvent to the Broker
+              We can send an http request directly to the [Broker](../../../broker-trigger.md) with correct CloudEvent headers set.

Contributor

abrennan89 Mar 19, 2020

Not we.....
"You can...using the correct..."

docs/eventing/samples/helloworld/helloworld-python/README.md

+                    exit
+                    ```
+              ### Verify that event is received by helloworld-python app
+              Helloworld-python app logs the context and the msg of the above event, and replies back with another event.

Contributor

abrennan89 Mar 19, 2020

msg -> message, do not abbreviate

"The...app"

docs/eventing/samples/helloworld/helloworld-python/README.md

+                Play around with the CloudEvent attributes in the curl command and the trigger specification to understand how [Triggers work](../../../broker-trigger.md#trigger).
+              ## Verify reply from helloworld-python app
+              `helloworld-python` app replies back with an event of `type= dev.knative.samples.hifromknative`, and `source=knative/eventing/samples/hello-world`. This event enters the eventing mesh via the Broker and can be delivered to other services using a Trigger

Contributor

abrennan89 Mar 19, 2020

Again, "The...app"
Also, be consistent when using backticks, formatting, capitalization for all naming. The app is referred to in different ways throughout the doc.

Contributor

abrennan89 Mar 19, 2020

Use "through", not "via"

Contributor Author

axsaucedo commented Mar 19, 2020

@abrennan89 thank you for the comments! Before diving into changing them I just want to point out that the actual content/writing is almost exactly the same as the current Golang example. There's seems to be a lot of wording / tone specific changes required here, but just want to confirm for concistency as all the comments provided here apply to your existing Golang helloworld example https://knative.dev/docs/eventing/samples/helloworld/helloworld-go/

What would be the best way forward here? Would it make sense to update the consistency of writing once there is guidelines for terminology? I can for the meantime apply the changes specific to the example such as the requirements.txt issue

Member

evankanderson commented Apr 10, 2020

/ok-to-test

knative-prow-robot removed the needs-ok-to-test label

Contributor

abrennan89 commented Jun 11, 2020

@axsaucedo I think having some sections as close to correct as we can for now is more important than trying to have consistency across everything. We're in the process of figuring out things like style guides / glossaries for docs, so they won't be consistent for a while, but we can start with small improvements like these 🙂

abrennan89 mentioned this pull request

Add abrennan89 to doc-approvers #2579

Merged

3 tasks

Contributor

abrennan89 commented Jul 8, 2020

@axsaucedo is this PR still WIP?

Contributor Author

axsaucedo commented Jul 8, 2020

@abrennan89 sorry for the radio silence, I don't think I'll be able to get around this unfortunately, would still be worth haivng a python example tho - thanks for the comments tho @abrennan89!

abrennan89 mentioned this pull request

Fix review comments from #2299 #2704

Closed

Contributor

abrennan89 commented Jul 28, 2020

/lgtm
/approve

knative-prow-robot added the lgtm label

Contributor

knative-prow-robot commented Jul 28, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, axsaucedo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~docs/eventing/OWNERS~~ [abrennan89]

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

knative-prow-robot added the approved label

knative-prow-robot merged commit 0b86a4a into knative:master

Contributor

vmexalien commented Sep 9, 2020

working on this as part of #2704

Contributor

vmexalien commented Sep 28, 2020 •

edited

Loading

line 148 - add comma for readability sample folder), you're ready
line 150 - reword Use Docker to build a container from the sample code.
line 162 - correct capitalization for consistency Docker Hub
line 170 - edit for readability; The above command
line 174 - another reference to "it"; The command deployed and change names to named

(these will be added in a new issue based on best practice)

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

Reviewers

evankanderson evankanderson left review comments

lionelvillard Awaiting requested review from lionelvillard

n3wscott Awaiting requested review from n3wscott

+3 more reviewers

mattmoor mattmoor left review comments

vmexalien vmexalien left review comments

abrennan89 abrennan89 requested changes

Labels

cla: yes lgtm ok-to-test size/L