Skip to content

Use SHOULD for TriggerSpec to keep compatibility #4838

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

Merged
knative-prow-robot merged 1 commit into from
Feb 3, 2021
Merged

Use SHOULD for TriggerSpec to keep compatibility #4838

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/spec/broker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ with that particular Broker, while the `TriggerSpec.Delivery`, if configured,
fully overrides `BrokerSpec.Delivery` for that particular Trigger, hence:

- When `BrokerSpec.Delivery` and `TriggerSpec.Delivery` are both not configured,
no delivery spec MUST be used.
no delivery spec SHOULD be used.
Copy link
Copy Markdown
Contributor

@vaikas vaikas Feb 4, 2021

Choose a reason for hiding this comment

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

wait, why are we changing all of these? I can see changing the TriggerSpec.Delivery but why all of them?

Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper Feb 4, 2021

Choose a reason for hiding this comment

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

That's fine, atm BrokerSpec.Delivery it's not compliant in all brokers (like in mtbroker)

Copy link
Copy Markdown
Contributor

@vaikas vaikas Feb 4, 2021

Choose a reason for hiding this comment

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

@slinkydeveloper @zhongduo I don't see why we needed to change all these to support backwards compatibility.

Copy link
Copy Markdown
Contributor Author

@zhongduo zhongduo Feb 4, 2021

Choose a reason for hiding this comment

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

I am not familiar with all the other brokers, but slinkydeveloper seems to make the point that this is not supported by all the brokers too. I am good with keeping the origin too, though we use MUST rarely.

Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper Feb 5, 2021

Choose a reason for hiding this comment

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

@vaikas do we want this spec to "map to the reality", or the "reality map to the spec"?

- When `BrokerSpec.Delivery` is configured, but not the specific
`TriggerSpec.Delivery`, then the `BrokerSpec.Delivery` MUST be used.
- When `TriggerSpec.Delivery` is configured, then `TriggerSpec.Delivery` MUST be
`TriggerSpec.Delivery`, then the `BrokerSpec.Delivery` SHOULD be used.
- When `TriggerSpec.Delivery` is configured, then `TriggerSpec.Delivery` SHOULD be
used.

## Data Plane
Expand Down