-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add docs / samples for Sequence #1481
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
Conversation
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| ### Create the Sequence | ||
|
|
||
| Here, if you are using different type of Channel, you need to change the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldnt that be always, w/ the new API/CRDs, there is no "default" channel, AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I mean if they don't want to use InMemoryChannel, say they want to use Kafka, they need to set their environment up differently.
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Outdated
Show resolved
Hide resolved
|
Thanks for the review, PTALl |
docs/eventing/samples/sequence/sequence-reply-to-sequence/README.md
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,194 @@ | |||
| --- | |||
| title: "Sequence Wired to event-display" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice write up, @vaikas-google ! I'll review this later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has a lot more than I realized! Here's my first round of edits; still have a little more I'd like to do, but wanted to get these over to you.
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Show resolved
Hide resolved
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Outdated
Show resolved
Hide resolved
docs/eventing/samples/sequence/sequence-reply-to-sequence/README.md
Outdated
Show resolved
Hide resolved
docs/eventing/samples/sequence/sequence-reply-to-sequence/README.md
Outdated
Show resolved
Hide resolved
docs/eventing/samples/sequence/sequence-with-broker-trigger/display-trigger.yaml
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews. I believe I addressed all the feedback, PTAL. |
|
/lgtm |
|
/hold |
| type: "docs" | ||
| --- | ||
|
|
||
| # Using Sequences in series |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can come up with a title that explicitly states the value of doing this -- what do you get from using sequences in a series? What use-case is this ideal for?
(This is the first time I've seen the term "Sequence" in the context of Eventing; could be really helpful to add links out to more info if this is a new object we've added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, add a link to the sequence page you created further down in this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add a backpointer to it, however, my thinking was that the landing page will be sequence.md and then the links point to specific examples like this.
Perhaps, I should switch Prerequisites with the Overview and then from Overview add the link to Sequence (../../../../sequence.md)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vaikas-google It's very hard to guarantee a particular reading path (thanks to Google search, people sharing links in Slack, people's own preferences when browsing, etc.) so it's best to add links to related content so that people can find the information no matter where they land.
I would be in favor of moving "Overview" up before "Prerequisites" and adding a link to the sequence page there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| ## Prerequisites | ||
|
|
||
| For this example, we'll assume you have set up an `InMemoryChannel` | ||
| as well as Knative Serving (for our functions). The examples use `default` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| as well as Knative Serving (for our functions). The examples use `default` | |
| as well as Knative Serving (for our functions). The examples use the `default` |
|
|
||
| For this example, we'll assume you have set up an `InMemoryChannel` | ||
| as well as Knative Serving (for our functions). The examples use `default` | ||
| namespace, again, if you want to deploy to another Namespace, you will need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| namespace, again, if you want to deploy to another Namespace, you will need | |
| namespace. If you want to deploy to another namespace, you will need |
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Show resolved
Hide resolved
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| ## Usage | ||
|
|
||
| ### Sequence Spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to include an example of a typical Sequence specification?
| @@ -0,0 +1,194 @@ | |||
| --- | |||
| title: "Sequence Wired to event-display" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has a lot more than I realized! Here's my first round of edits; still have a little more I'd like to do, but wanted to get these over to you.
docs/eventing/samples/sequence/sequence-reply-to-event-display/README.md
Outdated
Show resolved
Hide resolved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: samodell, vaikas-google 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:
Approvers can indicate their approval by writing |
* sequence with broker * add sequence -> sequence sample * sequence wired to event display * terminal sequence * first two examples moved to new v1beta1 services + cascading changes * fix sequence2sequence to use v1beta services / default ns * fix broker example to use v1beta1 service + default ns * Address PR feedback * add a pointer to vaikas-google/transformer sample * update transformer image refs to latest * address PR feedback * move overview before prerequisites, add link to main sequence page
Fixes: knative/eventing#1367
Proposed Changes