Skip to content

Conversation

shreyasiddhartha
Copy link
Contributor

Change type: Doc update; Connecting brokers to event sinks
Doc JIRA: RHDEVDOCS-5163

Fix Version: Openshift 4.12+

Doc Preview:

SME Review:
QE Review:
Peer Review:

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 27, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 27, 2023

@shreyasiddhartha: This pull request references RHDEVDOCS-5163 which is a valid jira issue.

In response to this:

Change type: Doc update; Connecting brokers to event sinks
Doc JIRA: RHDEVDOCS-5163

Fix Version: Openshift 4.12+

Doc Preview:

SME Review:
QE Review:
Peer Review:

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.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 27, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Apr 27, 2023

🤖 Updated build preview is available at:
https://59364--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/16523

@abrennan89 abrennan89 added branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 dev-tools Label for all Odo/Pipelines/Helm/Developer Console/Perspective PRs labels May 23, 2023
@abrennan89 abrennan89 added this to the Continuous Release milestone May 23, 2023
toc::[]

Brokers can be used in combination with triggers to deliver events from an event source to an event sink. Events are sent from an event source to a broker as an HTTP `POST` request. After events have entered the broker, they can be filtered by CloudEvent attributes using triggers, and sent as an HTTP `POST` request to an event sink.

Copy link
Contributor

Choose a reason for hiding this comment

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

The title of the assembly is Connecting brokers to sinks but all the 'Includes' talk about creating Triggers for which we have a separate section. Also, these modules are being reused from the Creating Triggers section, which IMHO is not needed.

The main work flow here, I think, is 'Delivering events from an event source to an event sink.'

@pierDipi @gabriel-rh please correct me if I am wrong, but here's my suggestion:

  1. We should pull out the Event Delivery section from Brokers and create a separate heading for it under Event discovery.
  2. We should add the content in this PR as the first module in the Event delivery section.
  3. Rename the module as 'Using brokers and triggers to deliver events'.
  4. Add an additional resources section in the module and link to https://docs.openshift.com/container-platform/4.13/serverless/eventing/triggers/create-triggers.html
  5. Make this module the second one: https://docs.openshift.com/container-platform/4.13/serverless/eventing/brokers/serverless-event-delivery.html#trigger-event-delivery-config_serverless-event-delivery. to sustain the flow.
  6. Keep the other sections as is.

@shreyasiddhartha thank you for your work. Let's wait for Pier to respond before you make the changes. Also, let me know if you would like to discuss this further.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhuss request your guidance please, especially considering the comment here: https://docs.google.com/spreadsheets/d/1eYAktpCGQaH5CHeGJ3W3RzQ-k2FvrjP_gLWymxTnMMU/edit#gid=428341185

Please correct me if I am wrong, the way I understand it is that event source, sink, broker, channel, subscription etc. are building blocks that can be used to create a customized event delivery master piece. Ideally our docs focus more on the user story and to my mind, event delivery seems to be the key workflow/user story in Eventing. If this is correct, then I suggest we bring the Event Delivery section on the level of Brokers etc.

I would further suggest that, if Shreya is ok taking this up, to move the event discovery section under the Event Sources section (make it an assembly under event sources) as all the modules under it deal with event sources and not general eventing. So ideally we would have the event delivery section after Subscriptions section.

Copy link
Contributor

@Preeticp Preeticp May 25, 2023

Choose a reason for hiding this comment

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

@shreyasiddhartha we need to add the exact preview link to the description/first comment of your PR to help reviewers: https://59364--docspreview.netlify.app/openshift-enterprise/latest/serverless/eventing/brokers/serverless-connecting-brokers-sinks.html

Copy link
Member

@pierDipi pierDipi May 25, 2023

Choose a reason for hiding this comment

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

For 1.

We should pull out the Event Delivery section from Brokers and create a separate heading for it under Event discovery.

I wouldn't put it under Event discovery since Event discovery is a different subject (and we will probably have more content added to it with the future features that we are working on upstream that are cross cutting all other resources).

Ideally our docs focus more on the user story and to my mind, event delivery seems to be the key workflow/user story in Eventing. If this is correct, then I suggest we bring the Event Delivery section on the level of Brokers etc.

I agree with this!

I would further suggest that, if Shreya is ok taking this up, to move the event discovery section under the Event Sources section (make it an assembly under event sources) as all the modules under it deal with event sources and not general eventing. So ideally we would have the event delivery section after Subscriptions section.

I wouldn't do this for now, since new features will have more cross cutting content

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, @pierDipi, so for now we:

  1. Pull out the Event Delivery section from Brokers and bring the Event Delivery section on the level of Brokers etc. and place it after Subscriptions section.
  2. Follow steps 2-5 as mentioned in my first comment.
  3. Let Event Delivery stay as is.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2023
- Name: Managing subscriptions
File: serverless-subs-managing
- Name: Event delivery
Dir: delivery
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
Dir: delivery
Dir: event-delivery

- Name: Managing subscriptions
File: serverless-subs-managing
- Name: Event delivery
Dir: delivery
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
Dir: delivery
Dir: event-delivery

* xref:../../../serverless/eventing/triggers/serverless-triggers.adoc#serverless-triggers[Triggers]
xref:../../../serverless/eventing/event-sources/knative-event-sources.adoc#knative-event-sources[Event sources]
* xref:../../../serverless/eventing/brokers/serverless-event-delivery.adoc#serverless-event-delivery[Event delivery]
* xref:../../../serverless/eventing/event-delivery/serverless-event-delivery-parameters-config.adoc#serverless-event-delivery[Event delivery]
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
* xref:../../../serverless/eventing/event-delivery/serverless-event-delivery-parameters-config.adoc#serverless-event-delivery[Event delivery]
* xref:../../../serverless/eventing/event-delivery/serverless-event-delivery-parameters-config.adoc#serverless-event-delivery-parameters-config[Event delivery]

ID needs to be updated I think because it was changed and the filename was changed

@abrennan89 abrennan89 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2023
@abrennan89 abrennan89 closed this Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 dev-tools Label for all Odo/Pipelines/Helm/Developer Console/Perspective PRs do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants