Use SHOULD for TriggerSpec to keep compatibility

[zhongduo]

Fixes #4837

This is to keep compatibility with brokers that cannot support per trigger delivery.

[zhongduo]

/cc @slinkydeveloper @vaikas @lionelvillard @grantr

[codecov]

Codecov Report

Merging #4838 (d755839) into master (1fa597e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4838   +/-   ##
=======================================
  Coverage   81.27%   81.27%           
=======================================
  Files         292      292           
  Lines        8309     8309           
=======================================
  Hits         6753     6753           
  Misses       1146     1146           
  Partials      410      410           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa597e...d755839. Read the comment docs.

[slinkydeveloper]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper, zhongduo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [slinkydeveloper,zhongduo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vaikas]

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

[slinkydeveloper]

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

[vaikas]

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

[zhongduo]

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.

[slinkydeveloper]

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

[vaikas]

I'm just saying that the original issue said that using MUST in triggerspec.delivery breaks backwards compatibility and should be reverted. That's what the issue said so was confused why we changed them all.

[vaikas]

As far as reality vs. what we want the spec to say, I'd think we define the spec and then we can either have conformant brokers or not. Just because a ref broker currently does not support the behaviour should not mean that we tailor to spec to reflect the current implementation.

[slinkydeveloper]

Ok then IMO we should revert as a whole this PR, since we want that brokers supports both TriggerSpec.Delivery and BrokerSpec.Delivery

[zhongduo]

Ok then IMO we should revert as a whole this PR, since we want that brokers supports both TriggerSpec.Delivery and BrokerSpec.Delivery

IMO in the whole spec MUST is used with extreme caution, as it will make all the previous broker not compatible. If we want this to be a breaking change, we need more discussion.

[slinkydeveloper]

IMO in the whole spec MUST is used with extreme caution, as it will make all the previous broker not compatible. If we want this to be a breaking change, we need more discussion.

And this goes back to what i've asked before: is this spec the goal of a broker impl? or it's meant to map the reality? Note that the change to the spec i made is not a breaking change (except for mtbroker), it just adds another feature which i believe every broker should implement