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

[WIP] Adding ODC SinkBinding docs #24166

Closed
wants to merge 1 commit into from

Conversation

abrennan89
Copy link
Contributor

@abrennan89 abrennan89 commented Jul 24, 2020

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

@cardil cardil left a comment

Choose a reason for hiding this comment

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

Some minor questions to be answered

modules/serverless-sinkbinding-kn.adoc Show resolved Hide resolved
modules/serverless-sinkbinding-odc.adoc Show resolved Hide resolved
modules/serverless-sinkbinding-odc.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@cardil cardil left a comment

Choose a reason for hiding this comment

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

Those verification steps will not work.


.Verification steps

You can verify that the Kubernetes events were sent to the Knative event sink by looking at the message dumper function logs.
Copy link
Contributor

@cardil cardil Aug 3, 2020

Choose a reason for hiding this comment

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

Unfortunately, this will not work. Previously we had here a weak verification, just by looking at how ODC renders those resources. Now, you've copied verification steps from other ways of configuring sinkbinding. This approach has couple of pitfalls:

  1. We don't setup here any event producer. That means that user will not see any event on event-display logs. We could fix that, by deploying some event producer like cronjob or pingsource. Best will be to deploy them using ODC only.
  2. We use CLI tools to verify ODC operation. User may not have CLI configured. That's why "ODC way" may have been chosen to follow. We could fix that by rewriting this CLI based verification to one that uses ODC UI to check the same outcome.
  3. I'd left previous paragraph of how ODC should render sinkbinding being connected to ksvc, just as first step of simple verification.

Copy link
Member

Choose a reason for hiding this comment

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

User can click om Knative Service in topology and on sidebar , it'll show Pods with view logs option, user can click on view logs and check the events in logs similar to what's being done via CLI. @cardil Thoughts?

@Preeticp
Copy link
Contributor

Preeticp commented Aug 4, 2020

@invincibleJai can you please review this PR and provide an SME ack?

@invincibleJai
Copy link
Member

@abrennan89 if this Doc is from 4.6 POV , then images needs to be updated and SinkBinding got it's icon

image

@Preeticp
Copy link
Contributor

Preeticp commented Aug 4, 2020

Hey @abrennan89 I just had a brief look and have asked Jai the ODC-Serverless SME to take a look at it in addition to the ODC QE, before I do a peer review. For Pipelines, we have QE for both ODC and Pipelines review docs for ODC-Pipelines, it probably makes sense for us to follow a similar model for ODC-Serverless issues too. WDYT?

Apart from that, is there a reason why we have this section in the Knative Eventing Sub-bucket instead of the Event sources Sub-bucket, before or after: https://sinkbindodc--ocpdocs.netlify.app/openshift-enterprise/latest/serverless/event_sources/serverless-apiserversource.html

Comment on lines +41 to +42
... For **apiVersion** enter `v1`.
... For **Kind**, enter `SinkBinding`.
Copy link
Member

Choose a reason for hiding this comment

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

here apiVersion and kind will be kind of subject as in below deployment can even have Job
See label selector to have it working even Deployment needs to be created to proper labels

https://github.com/knative/docs/blob/master/docs/eventing/samples/sinkbinding/sinkbinding.yaml#L19-L25

spec:
  subject:
    apiVersion: apps/v1
    kind: Deployment
    selector:
      matchLabels:
        app: wss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gonna revisit this and use the CronJob / heartbeats example for now.

We can't really doc every option for kind under this PR at least. There are general docs here about CRDs / kind: https://docs.openshift.com/container-platform/4.5/operators/crds/crd-extending-api-with-crds.html#crd-custom-resource-definitions_crd-extending-api-with-crds
but they don't mention much about specific types. Maybe there should be a link to https://kubernetes.io/docs/concepts/workloads/controllers/ or similar that explains these concepts without us having to list all the supported types?

@abrennan89
Copy link
Contributor Author

abrennan89 commented Aug 4, 2020

@abrennan89 if this Doc is from 4.6 POV , then images needs to be updated and SinkBinding got it's icon

@invincibleJai these docs are for OCP 4.5; they might need an update for 4.6 but there's an issue open for updating the images for that so I would say 4.6 updates should be done under https://issues.redhat.com/browse/RHDEVDOCS-2085

@abrennan89
Copy link
Contributor Author

Hey @abrennan89 I just had a brief look and have asked Jai the ODC-Serverless SME to take a look at it in addition to the ODC QE, before I do a peer review. For Pipelines, we have QE for both ODC and Pipelines review docs for ODC-Pipelines, it probably makes sense for us to follow a similar model for ODC-Serverless issues too. WDYT?

Followed up in Slack.

Apart from that, is there a reason why we have this section in the Knative Eventing Sub-bucket instead of the Event sources Sub-bucket, before or after: https://sinkbindodc--ocpdocs.netlify.app/openshift-enterprise/latest/serverless/event_sources/serverless-apiserversource.html

It's because sinkbinding technically isn't an event source, so our docs were separate, but I can move it to the event sources section since that's where it is in the UI now 🤷

@cardil
Copy link
Contributor

cardil commented Aug 4, 2020

It's because sinkbinding technically isn't an event source, so our docs were separate, but I can move it to the event sources section since that's where it is in the UI now shrug

For me SinkBinding is in the same class of "thing" as channels. It's an event's transport, a bridge, a road, on which events are delivered from place A to place B. Channels are similar, but events routes are decided by using triggers. Analogy would be a road junction with road sings that forces different type of cars to take different lanes.

Those are a "transport" class things. Event producers are "things" that creates and send events. Taking the analogy one step further, it may be a car dealer, a garage, etc.

@Preeticp
Copy link
Contributor

Preeticp commented Aug 4, 2020

Thanks for explaining that @abrennan89 I get it now. I came at it from the UI pov(screenshot) :)
Please organize it as you see fit.

@abrennan89
Copy link
Contributor Author

Closing as per discussions with team and https://issues.redhat.com/browse/RHDEVDOCS-1950

@abrennan89 abrennan89 closed this Aug 5, 2020
@abrennan89 abrennan89 deleted the sinkbindODC branch December 17, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

None yet

6 participants