Eventing: Unused Delivery in the ChannelableSpec

[slinkydeveloper]

Describe the bug
We have a Delivery field in the ChannelableSpec, but it's orphan: nothing is using this field.

Expected behavior
:question:

Additional context
What should we do with this field?

• Implement it
  
  
  
  • Where? on the duck type controllers? on the downstream channel controllers?
  
  
• Remove it

[devguyio]

@lionelvillard do you have any issue, PR or document that has some context around this? Or do you remember it being discussed in the Channel taskforce meetings?
/cc @Harwayne @matzew

[aliok]

I actually like the idea of defining the delivery spec in the channel level and with an option to be able to override them in the subscriptions. ...but what I understand is that this is never implemented.

[aliok]

Now I checked the 0.15 and 0.16 branches to see if it was used before and if we mistakenly deleted its usages while updating the reconcilers. But, nope, it was not used previously as well.

[matzew]

But, nope, it was not used previously as well.

I don't have exact references, but recall that @aliok had some problems regarding this

[aliok]

/assign

I will try to gather some information about this one:

• Is this there because of the contract in the spec doc?
• No wants this feature or nobody wants to implement it?

[aliok]

Some info:

• This was introduced some time ago to support the idea of defining the delivery config in the channel and being able to override it in the subscriber level
• The channel spec doesn't talk about this: https://github.com/knative/eventing/blob/master/docs/spec/channel.md/spec.md#kind-channel, the API reference also doesn't talk about this: https://github.com/knative/eventing/blob/master/docs/spec. So, this was not added to conform a spec or something.

So, we should either implement the feature that's supposed to use the Delivery or delete this field in v2alpha1.

[slinkydeveloper]

@aliok since nobody implemented it, nobody is using it, so perhaps can we just remove it from v1 apis?

[aliok]

That's also an option. I will bring this in today's Event Delivery WG.

[Harwayne]

@aliok since nobody implemented it, nobody is using it, so perhaps can we just remove it from v1 apis?

I don't think we can remove it from the v1 API.

---

In the Event Delivery we WG agreed to implement this, rather than removing it from the API. The first step will be defining exactly how the Delivery Specs interact with each other. E.g. if there is a delivery spec on the Channel and the Subscription, does one override the either entirely? Do they merge? If so, how? Similarly, if there is a DeliverySpec on a Broker/Trigger, can someone override the DeliverySpec on the underlying Channel/Subscription? Or does the Broker/Trigger completely own the underlying resource and will undo any DeliverySpec changes?

[lberk]

Closing for now as it seems the resolution is to not remove it (just a small matter of implementing it :) )

[slinkydeveloper]

@lberk should we keep it open up until it's implemented?

[slinkydeveloper]

/assign

I'm going to adjust the spec

[slinkydeveloper]

Spec: #4650
Implementation: #4652