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.

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

Add note to channel spec regarding channel subscription spec #3132

Merged
merged 5 commits into from
May 15, 2020
Merged

Add note to channel spec regarding channel subscription spec #3132

merged 5 commits into from
May 15, 2020

Conversation

lberk
Copy link
Member

@lberk lberk commented May 12, 2020

The spec.subscription fields shouldn't be set directly, rather
it should be set via the subscriptions themselves and then the status
should be propogated via the channel.

Related to:
#3050
#3051

@lberk
Add note to channel spec regarding channel subscription spec
c7db555 
The spec.subscription fields shouldn't be set directly, rather
it should be set via the subscriptions themselves and then the status
should be propogated via the channel.
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 12, 2020
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 12, 2020
@lberk
Copy link
Member Author

lberk commented May 12, 2020

/cc @aliok
/area channels
/area docs
/area iCantEvenSeeTheLabelsIHopeThoseWereRight

@knative-prow-robot knative-prow-robot requested a review from aliok May 12, 2020 14:31
@knative-prow-robot
Copy link
Contributor

@lberk: The label(s) area/docs, area/icantevenseethelabelsihopethosewereright cannot be applied, because the repository doesn't have them

In response to this:

/cc @aliok
/area channels
/area docs
/area iCantEvenSeeTheLabelsIHopeThoseWereRight

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.

@matzew
Copy link
Member

matzew commented May 12, 2020

/woof

@knative-prow-robot
Copy link
Contributor

@matzew: dog image

In response to this:

/woof

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.

@matzew
Copy link
Member

matzew commented May 12, 2020

icantevenseethelabelsihopethosewereright

🤣

aliok
aliok reviewed May 12, 2020
View reviewed changes
docs/spec/channel.md Outdated
Comment on lines 185 to 186
Note: the array of subscribers MUST NOT be set directly on the channel, but
rather appended by the subscription itself.
Copy link
Member

Choose a reason for hiding this comment

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

This means, we need to disable direct setting on IMC and other channels in the code as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aliok -- ok I've updated, please let me know what you think!

mattmoor
mattmoor reviewed May 14, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

@lberk @mattmoor
linting suggestion change
7465c76 
Co-authored-by: Matt Moore <mattmoor@vmware.com>
aliok
aliok approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@aliok aliok left a comment

Choose a reason for hiding this comment

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

/approve

Looks good @lberk

@lberk
Copy link
Member Author

lberk commented May 14, 2020

@Harwayne any concerns on the wording?

@vaikas
Copy link
Contributor

vaikas commented May 14, 2020

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, lberk, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2020
Harwayne
Harwayne reviewed May 14, 2020
View reviewed changes
docs/spec/channel.md Outdated Show resolved Hide resolved
@lberk @Harwayne
change CR to custom object
2837ca2 
Co-authored-by: Adam Harwayne <harwayne@google.com>
Harwayne
Harwayne reviewed May 15, 2020
View reviewed changes
Copy link
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/lgtm

mattmoor
mattmoor reviewed May 15, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
mattmoor
mattmoor reviewed May 15, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

docs/spec/channel.md Outdated Show resolved Hide resolved
@lberk @mattmoor
more linting
e53fb4d 
Co-authored-by: Matt Moore <mattmoor@vmware.com>
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
@Harwayne
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
@knative-prow-robot knative-prow-robot merged commit b3a3da6 into knative:master May 15, 2020
@aliok aliok mentioned this pull request May 15, 2020
Closed
@aliok aliok mentioned this pull request Sep 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor left review comments

@Harwayne Harwayne Harwayne left review comments

@aliok aliok aliok approved these changes

@yolocs yolocs Awaiting requested review from yolocs

Assignees

@Harwayne Harwayne

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/channels cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lberk @knative-prow-robot @matzew @vaikas @Harwayne @aliok @mattmoor @googlebot