Fix spec #4515

[slinkydeveloper]

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

PR to modify the spec according to #4515

Proposed Changes

• 🎁 Add the Trigger.Delivery field
• 🧹 Now Broker.Delivery defaults the Trigger.Delivery field. Note: this is a breaking change for mtbroker users that are using the Broker.Delivery field, because this field, even if it remains the same, will change meaning for them (in a followup I'll perform the actual change)

Release Note

:gift: Add Trigger.Delivery field which allows configuration of Delivery per Trigger.

Docs

[lberk]

@slinkydeveloper just reviewing PR's from before the break, has this been brought up in the delivery WG?

[slinkydeveloper]

@lberk It was on 15/12/20, but there were just a bunch of people there

[pierDipi]

/kind enhancement
/kind api-change

[matzew]

What's the migration strategy for users here ? Asking b/c we have v1 API shape here.

Let's define a proper one... or move this to something like v2alpha1 ?

/hold

[slinkydeveloper]

Let's define a proper one... or move this to something like v2alpha1 ?

This is an actual bug of the v1 spec. To be specific, the field BrokerSpec.Delivery remains, but changes the meaning to avoid leaking internal details (which is the meaning people assumed downstream). We could just keep the actual mtbroker implementation, but then it would be a non-compliant implementation i guess?

What's the migration strategy for users here ? Asking b/c we have v1 API shape here.

I think we can create a script for mtbroker users that just moves the delivery field for them to somewhere else

[matzew]

/unhold

thanks for addressing my comments, Slinky

[vaikas]

I just want to understand how this is supposed to work with the wording about defaulting fields. It's more of an implementation question but has implications on when the defaulting happens and if it even should happen. The PR description seems to say that we default the triggerspec.delivery field, so if that's how we're going to implement it then it matters when the defaulting applies. If however you implement it by not actually updating the triggerspec.delivery but look at the brokerspec.delivery if triggerspec.delivery is missing then the behaviour is different.

In particular, it would be good to clarify that, for example, what happens in the following scenarios:

1. Create Broker, no delivery

2. Create Trigger, no delivery

3. Update Broker, configure delivery, what happens here?

4. Create Broker, with delivery

5. Create Trigger, no delivery -> defaulting happens

6. Update Broker, delete/update broker delivery -> what happens to trigger above

7. Create Trigger2, no delivery, will it match what Trigger from 2 has?

Just want to understand that behaviour little better about what "defaulting" really means and what the expected behaviour is.
As I said, depending on how this is implemented and if we actually modify the triggerspec.delivery vs. treating missing triggerspec.delivery as 'use brokerspec.delivery' has different semantics potentially and certainly makes the implementation more tricky depending on how it's implemented.

[vaikas]

From the PR description, this is what I'm trying to understand with the description on the spec.

Now Broker.Delivery defaults the Trigger.Delivery field. Note: this is a breaking change for mtbroker users that are using the Broker.Delivery field, because this field, even if it remains the same, will change meaning for them (in a followup I'll perform the actual change)

[slinkydeveloper]

Defaulting basically means that the trigger delivery spec, if any, wins on the broker delivery spec, exactly as it works today with subscription and channel delivery fields. So the result of the above cases:

2. defaults to Broker.Delivery -> no delivery

3. defaults to Broker.Delivery -> delivery from Broker.Delivery

4. defaults to Broker.Delivery -> delivery from Broker.Delivery

5. defaults to Broker.Delivery -> delivery from Broker.Delivery (no delivery if Broker.Delivery was removed)

6. defaults to Broker.Delivery -> delivery from Broker.Delivery (no delivery if Broker.Delivery was removed)

Another case:

8. Create Trigger3, with delivery -> delivery from Trigger3.Delivery

[slinkydeveloper]

I can provide the implementation for mtbroker to show how that works, but I don't see any particular implementation issues with it

[vaikas]

Right, so I think it was (as I tried to say) how it's worded about "defaulting" since typically defaulting means filling in the fields and if that's how you were going to implement it, it could get confusing. If you remove the word defaulting I think I'd be much happier. What I tried to say :) was that spec seems ok, but your PR description confused me wrt. using words defaulting.
If you say:
If triggerspec.delivery is specified it is used. If triggerspec.delivery does not exist brokerspec.delivery is used. Nothing about defaulting :)

[slinkydeveloper]

How about now? I removed the default word everywhere and added some wording to the spec

[vaikas]

If we backport this to v1 I'm thinking we should also backport it to v1beta1 as well. Looks like the roundtrippers are failing when it's trying to go from v1->v1beta1->v1 and the field is not surviving that. So I think:

1. Add this to v1beta1 as well
2. Add conversion to/from
   WDYT?

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1/trigger_validation.go	96.4%	93.2%	-3.2
pkg/apis/eventing/v1beta1/trigger_conversion.go	100.0%	93.9%	-6.1
pkg/apis/eventing/v1beta1/trigger_validation.go	96.1%	92.6%	-3.5

[codecov]

Codecov Report

Merging #4654 (d196f91) into master (ae17ca3) will decrease coverage by 0.10%.
The diff coverage is 54.28%.

@@            Coverage Diff             @@
##           master    #4654      +/-   ##
==========================================
- Coverage   81.25%   81.15%   -0.11%     
==========================================
  Files         291      292       +1     
  Lines        8281     8311      +30     
==========================================
+ Hits         6729     6745      +16     
- Misses       1143     1152       +9     
- Partials      409      414       +5     

Impacted Files	Coverage Δ
pkg/apis/eventing/v1/broker_types.go	66.66% <ø> (ø)
pkg/apis/eventing/v1/trigger_types.go	100.00% <ø> (ø)
pkg/apis/eventing/v1/trigger_validation.go	86.30% <0.00%> (-3.70%)	⬇️
pkg/apis/eventing/v1beta1/trigger_types.go	100.00% <ø> (ø)
pkg/apis/eventing/v1beta1/trigger_validation.go	84.84% <0.00%> (-4.05%)	⬇️
pkg/apis/eventing/v1beta1/trigger_conversion.go	89.18% <50.00%> (-10.82%)	⬇️
pkg/channel/message_dispatcher.go	75.92% <62.50%> (-1.40%)	⬇️
pkg/channel/attributes/knative_error.go	100.00% <100.00%> (ø)

---

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 ae17ca3...a43b49d. Read the comment docs.

[slinkydeveloper]

It seems like downstream tests are passing but failing in the license phase, for some reason not related to this PR:

  Error: go [-e -json -compiled=false -test=false -export=false -deps=true -find=false -- ./...]: exit status 1: go: inconsistent vendoring in /home/runner/work/eventing/eventing/src/knative.dev/eventing-kafka:

[vaikas]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,vaikas]

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