Skip to content

Update docs to v1beta1 + add more details about channel based broker #2259

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
merged 4 commits into from
Mar 3, 2020
Merged

Update docs to v1beta1 + add more details about channel based broker #2259

merged 4 commits into from
Mar 3, 2020

Conversation

@vaikas
Copy link
Contributor

@vaikas vaikas commented Feb 27, 2020

Addresses #2248

Proposed Changes

  • Update documents for Broker / Trigger for v1beta1.
  • Add a more detailed section for Channel Based Broker and split out the installation from usage.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 27, 2020
@knative-prow-robot knative-prow-robot added approved size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2020
@vaikas
Copy link
Contributor Author

vaikas commented Feb 28, 2020

I think the missing link should be fixed by: #2126

grantr
grantr reviewed Feb 28, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Thanks for updating this @vaikas!

docs/eventing/channel-based-broker.md
kubectl -n foo delete broker default
```

## Installing Broker Manually
Copy link
Contributor

@grantr grantr Feb 28, 2020

Choose a reason for hiding this comment

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

Honestly I think this section should be removed. Why would a user ever do this if they can annotate the namespace (and if they can create ServiceAccounts, they can probably annotate the namespace)? Maybe a contributor needs to know, but then it can be part of the eventing repo.

Copy link
Contributor Author

@vaikas vaikas Feb 28, 2020

Choose a reason for hiding this comment

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

Yeah, I initially thought about removing it entirely, but for the cases where you might have multiple types of brokers in a given namespace, you can't just label the namespace. You could for example have a Broker foo and bar, and you would need to create one of them manually.

docs/eventing/channel-based-broker.md
The `ServiceAccount` was created two commands prior. The `RoleBinding` is
created with this command.

Create RBAC permissions granting access to shared configmaps for logging,
Copy link
Contributor

@grantr grantr Feb 28, 2020

Choose a reason for hiding this comment

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

These instructions can be replaced with a ConfigMapPropagation (that's what the namespace controller uses now)

Copy link
Contributor Author

@vaikas vaikas Feb 28, 2020

Choose a reason for hiding this comment

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

Is this something that you or @grac3gao could do after we get the initial changes in?

@vaikas
Copy link
Contributor Author

vaikas commented Feb 28, 2020

Thanks @grantr PTAL.

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Mar 3, 2020

@vaikas: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-docs-markdown-link-check a6afdea link /test pull-knative-docs-markdown-link-check

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

grantr
grantr approved these changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020
@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Mar 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, vaikas

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit e0657a3 into knative:master Mar 3, 2020
@abrennan89 abrennan89 mentioned this pull request Mar 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lionelvillard lionelvillard Awaiting requested review from lionelvillard

@n3wscott n3wscott Awaiting requested review from n3wscott

1 more reviewer

@grantr grantr grantr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@grantr grantr

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vaikas @knative-prow-robot @grantr @googlebot