Skip to content
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.

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

Add to pingsource.md #3476

Closed
wants to merge 12 commits into from
Closed

Add to pingsource.md #3476

wants to merge 12 commits into from

Conversation

RichardJJG
Copy link
Contributor

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 19, 2021
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 19, 2021
type: "docs"
aliases:
- ../cronjob-source
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be an absolute link, e.g. /docs/whatever/path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given no alias was present before (I think it was added through some merging accident) I'll simply delete it.


The PingSource source type is enabled by default when you install Knative Eventing.
## Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like overview as a heading - can we just move all the context info to above the "before you begin" instead? That way we're also not mixing steps / instructions and contextual info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

Comment on lines 20 to 21
In event-driven architecture the most straightforward use case is a producer creating an event and a
consumer acting on that event.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be repeated since we have "This topic explains event-driven architecture..." above. I'd choose one and remove the other. I think the previous statement is better but its up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will do.

Comment on lines +23 to +32
### Producers

### Creating a namespace
Producers create events. Using GitHub as an example, the producer is the GitHub webhook that sends
information when a pull request is created.
Essentially, you ask the producer for events that they are capable of sending and you tell the
producer where to send those events.

Create a new namespace called `pingsource-example` by entering the following
command:
Producers send the event data per specifications that make it easy for consumers to handle it.
Knative uses the CloudEvents specifications by default.
For more information, see the [Cloud Events](https://cloudevents.io/) website.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think this belongs here honestly, I think all of this info about event-driven etc should be in a main, generic section about event sources, not preceding a procedure on creating an example PingSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omerbensaadon wants it here, AFAIK. We could (should?) re-title this as more of a Getting Started document than a reference topic for PingSource buried way down the subnav.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abrennan89, it's good content regardless. Where would you rather it be placed?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://knative.dev/development/concepts/ maybe? Or the main eventing landing page, but both of those require cleanup first. FWIW there's a task for producers and consumers as part of #1923, so I'd do it as part of that rather than adding to pingsource.

Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have a place for this text right now, would it unblock this PR to add a <!-- TODO(<issue link>): move this to conceptual documentation when that exists --> here?


To deploy the `event-display` consumer to your cluster, run the following
command:
## Create a consumer (Knative service)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use parentheses in headings, especially since there are many types of consumers not just a ksvc.
It's explained that it's a service in the next paragraph so there's no need to have it here IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

Comment on lines 44 to 45
To create a consumer, and verify that `PingSource` is working, create a simple Knative service that
receives CloudEvents and writes them to stdout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting - either use PingSource or PingSource, cc @omerbensaadon for style guide. Again, if we're going with the k8s, I'd leave out the backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll drop the backticks.

Comment on lines 44 to 61
replicas: 1
selector:
matchLabels: &labels
app: event-display
template:
metadata:
labels: *labels
spec:
containers:
- name: event-display
image: gcr.io/knative-releases/knative.dev/eventing-contrib/cmd/event_display
- image: gcr.io/knative-releases/knative.dev/eventing-contrib/cmd/event_display
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove all of this instead of explaining it?
Also, is it not event-display? I could be wrong but would check this with eng.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I received the much abridged example from the Google doc the engineer wrote; I've not deviated from what the engineer wants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, good catch -- @vaikas, this should be event-display rather than event_display, right?

targetPort: 8080
EOF
```shell
kn service create event-display --image gcr.io/knative-releases/knative.dev/eventing-contrib/cmd/event_display
Copy link
Contributor

Choose a reason for hiding this comment

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

again, I'd check this image isn't event-display rather than event_display

Copy link
Contributor Author

Choose a reason for hiding this comment

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

event-display rather than event_display, right, @vaikas?

{{< /tab >}}
{{< /tabs >}}

## Create a producer (PingSource)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, remove parentheses, I don't think there's a need to even mention concepts like producer vs consumer in this doc honestly w/ the exception of if you're explaining them as concepts. I'd just say "Create a PingSource"

@omerbensaadon can we also decide whether to use gerunds in headings please in the style guide. It might be nice if we could have something like the IBM style guide IMO as a reference but idk how licensing and funding for that would work upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to add just enough conceptual information to put the reader at better ease with the procedure. I can drop the parentheses, though. I like them as a hand-holding way of walking through what's actually going on, but I can see how the reader might think PingSource is the de facto producer.

Comment on lines 77 to 80
Create a producer (PingSource) that sends events to your consumer every two minutes.
The `sink` element in the metadata describes where to send events.
In this case, events are sent to a service with the name `event-display`.
This is a tight connection between the producer and consumer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, remove parentheses. Feels over-explained.
I don't think these all need to be separate lines either, it could just be a paragraph? The flow just feels off here IMO.

I think in general this includes too much context between steps of a procedure - IMO all the conceptual info should go in a generic event sources concept doc and the steps should be in a simple procedural topic.
Also, rather than explaining things like sink here, can we use the callout workaround style that was established; https://docs.google.com/document/d/1tigolRzKhronvgoAJfszE-XHWWd0LQ8o_j0D73O9KvM/edit#heading=h.ka7avn97fckt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll cut back a bit.

```

{{< /tab >}}
{{< /tabs >}}

## (Optional) Create a PingSource with binary data
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this its own section instead of being optional in an example. By the time a reader gets to here they've created a PingSource with the other methods. Also, please no more parentheses in titles 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense, thanks.


Please note that `data` and `dataBase64` cannot co-exist.
**Note:** `data` and `dataBase64` cannot co-exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Note:** `data` and `dataBase64` cannot co-exist.
**NOTE:** You can use either `data` or `dataBase64`, but not both.

Explain why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lionelvillard, why can't you use both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Both data and dataBase64 specify the CloudEvent payload, that's why those fields are mutually-exclusive.

Does that answer your question?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the following text, to clarify why:

**NOTE:** Because both `data` and `dataBase64` specify the event payload, only one field may be set.

Please note that `data` and `dataBase64` cannot co-exist.
**Note:** `data` and `dataBase64` cannot co-exist.

Create the event source with binary data from stdin by running:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just use PingSource throughout > event source

I think things like stdin that refer to something specific should always be in back ticks, and I really think we should explain this cos I don't know what it is. At least maybe add a link explaining it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added code formatting rules to the Style Guide, which people are free to support/dispute.

{{< tabs name="view-event" default="kubectl" >}}
{{% tab name="kubectl" %}}

View the logs of the event-display service by running:
Copy link
Contributor

Choose a reason for hiding this comment

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

Be consistent with names - event-display

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget the backticks, not sure why these were removed

### Cleanup
## Clean up

Delete the PingSource instance:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this because it's duplicated in the tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll delete the duplicate within the tabs.

@knative-prow-robot 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 Apr 22, 2021
@abrennan89
Copy link
Contributor

Re the comments above, can we go with "by entering the command:" rather than "by running"? Otherwise a good point about accessibility @RichardJJG

@knative knative deleted a comment from RichardJJG Apr 22, 2021
@knative knative deleted a comment from lionelvillard Apr 22, 2021
@RichardJJG
Copy link
Contributor Author

RichardJJG commented Apr 27, 2021

Re the comments above, can we go with "by entering the command:" rather than "by running"? Otherwise a good point about accessibility @RichardJJG

Why do you prefer the wordier "by entering the command:", @abrennan89?
I've written a "Documenting Commands" section in the style guide, by the way, that makes the case for "running". We can change it to "by entering the command" if that's the better option.

@abrennan89
Copy link
Contributor

@RichardJJG I think they're both equally valid and it's mostly personal preference (FWIW I'm not sure there's a better case for using "run" based on what's in the style guide atm?), however I'd prefer:
"To [do X], enter the command:"
over “X the Y by running:”

We should probably base a decision on what would be easier for non-native speakers / translation down the road, which I believe would be "enter", but we probably need some research to make an informed decision about that. @omerbensaadon can you help with this?

@knative-prow-robot 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 Apr 27, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign n3wscott after the PR has been reviewed.
You can assign the PR to them by writing /assign @n3wscott in a comment when ready.

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

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

1 similar comment
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign n3wscott after the PR has been reviewed.
You can assign the PR to them by writing /assign @n3wscott in a comment when ready.

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

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

Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

Added comments.

Q: Was the install stuff added to this PR by mistake?

Comment on lines +23 to +32
### Producers

### Creating a namespace
Producers create events. Using GitHub as an example, the producer is the GitHub webhook that sends
information when a pull request is created.
Essentially, you ask the producer for events that they are capable of sending and you tell the
producer where to send those events.

Create a new namespace called `pingsource-example` by entering the following
command:
Producers send the event data per specifications that make it easy for consumers to handle it.
Knative uses the CloudEvents specifications by default.
For more information, see the [Cloud Events](https://cloudevents.io/) website.
Copy link
Contributor

Choose a reason for hiding this comment

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

https://knative.dev/development/concepts/ maybe? Or the main eventing landing page, but both of those require cleanup first. FWIW there's a task for producers and consumers as part of #1923, so I'd do it as part of that rather than adding to pingsource.


```shell
kubectl create -n pingsource-example -f - <<EOF
apiVersion: sources.knative.dev/v1beta2
cat <<EOF | kubectl create -f -
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


Please note that `data` and `dataBase64` cannot co-exist.
**Note:** `data` and `dataBase64` cannot co-exist.
Copy link
Contributor

Choose a reason for hiding this comment

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


See the [PingSource specification](../../reference/api/eventing/#sources.knative.dev/v1beta2.PingSource).
Run:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


## Prerequisites

Before installation, you must meet the following prerequisites:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Before installation, you must meet the following prerequisites:
Before you install Knative, ensure that:

Just a less wordy suggestion

Comment on lines 36 to 38
## Installing the Serving component

To install the serving component:
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought per the word list we're using Knative Serving throughout now?

kubectl set env --namespace ambassador deployments/ambassador AMBASSADOR_KNATIVE_SUPPORT=true
```

1. To configure Knative Serving to use Ambassador by default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. To configure Knative Serving to use Ambassador by default:
1. To configure Knative Serving to use Ambassador by default, enter the command:

Copy link
Contributor

Choose a reason for hiding this comment

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

All the procedure steps in this part don't seem to have "run" or "enter" the command. In future I'd maybe keep this as a separate PR to the pingsource stuff, i.e. keep topics separated so it doesn't blur the PR scope

Comment on lines 488 to 490
## Installing the Eventing component

To install the Eventing component:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, Knative Eventing throughout?

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/meow

For an extra "unnecessary cat".


Please note that `data` and `dataBase64` cannot co-exist.
**Note:** `data` and `dataBase64` cannot co-exist.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the following text, to clarify why:

**NOTE:** Because both `data` and `dataBase64` specify the event payload, only one field may be set.


Please note that `data` and `dataBase64` cannot co-exist.
Create the event source with binary data from standard in by running:
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, I might call this "stdin", "standard input" (weird, but what STDIN is abbreviated from) or "terminal input".

Comment on lines +23 to +32
### Producers

### Creating a namespace
Producers create events. Using GitHub as an example, the producer is the GitHub webhook that sends
information when a pull request is created.
Essentially, you ask the producer for events that they are capable of sending and you tell the
producer where to send those events.

Create a new namespace called `pingsource-example` by entering the following
command:
Producers send the event data per specifications that make it easy for consumers to handle it.
Knative uses the CloudEvents specifications by default.
For more information, see the [Cloud Events](https://cloudevents.io/) website.
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have a place for this text right now, would it unblock this PR to add a <!-- TODO(<issue link>): move this to conceptual documentation when that exists --> here?


## Verify the message was sent

Verify that the message was sent to the Knative Eventing system by looking at message dumper logs.
Copy link
Member

Choose a reason for hiding this comment

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

I was going to point out that Knative Services may scale down to zero and have their logs recycled, but it occurs to me that the PingSource might be firing just frequently enough to avoid this.

I suspect this will work whether or not the service is getting scaled down to zero because there will be at least one pod running with at least one request in the logs (because the pingsource keeps sending events), so this is mostly a note for other examples.

kail -l serving.knative.dev/service=event-display -c user-container --since=10m
```

For more information, see the [`kail`](https://github.com/boz/kail) repository in GitHub.
Copy link
Member

Choose a reason for hiding this comment

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

+1

You could also reference using log aggregation via https://knative.dev/docs/install/collecting-logs/ as another way to view the logs if desired, but I'm not super-happy with that yet so I don't want to add it to this PR.

@knative-prow-robot
Copy link
Contributor

@evankanderson: cat image

In response to this:

/meow

For an extra "unnecessary cat".

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

@RichardJJG: PR needs rebase.

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 knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2021
@abrennan89
Copy link
Contributor

Agreed with Richard to close this one in favor of #3509
TODO: I'll add another PR to move the conceptual info for producers etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants