Rename `subscription.reply.channel` to `subscription.reply.sink`

[rhuss]

Problem

The reply strategy for a subscription can be any addressable:

// ReplyStrategy specifies the handling of the SubscriberSpec's returned replies.
// If no SubscriberSpec is specified, the identity function is assumed.
type ReplyStrategy struct {
	// You can specify only the following fields of the ObjectReference:
	//   - Kind
	//   - APIVersion
	//   - Name
	//  The resource pointed by this ObjectReference must meet the Addressable contract
	//  with a reference to the Addressable duck type. If the resource does not meet this contract,
	//  it will be reflected in the Subscription's status.
	// +optional
	Channel *corev1.ObjectReference `json:"channel,omitempty"`
}

However, the field in the strategy is still called "Channel". I suggest to rename it to "Sink" resulting in a field subscription.reply.sink instead of subscription.reply.channel, which would also align nicely with the naming scheme of source CRDs.

[matzew]

I think that is from the time of the first spec rewrite (Oct 2018), where we just had channels there. So a returned value from the subscriber of a subscription would be routed to the reply channel.

[rhuss]

I also found that PR which widened the scope of this field, but did not rename the key for some reason --> #1283

[lionelvillard]

For Sequence and Parallel, Reply is an ObjectReference:

type SequenceSpec struct {
	
	// Reply is a Reference to where the result of the last Subscriber gets sent to.
	//
	// You can specify only the following fields of the ObjectReference:
	//   - Kind
	//   - APIVersion
	//   - Name
	//
	//  The resource pointed by this ObjectReference must meet the Addressable contract
	//  with a reference to the Addressable duck type. If the resource does not meet this contract,
	//  it will be reflected in the Subscription's status.
	// +optional
	Reply *corev1.ObjectReference `json:"reply,omitempty"`
}

So maybe we should just get rid of ReplyStrategy.

[matzew]

Than the ReplyStrategy itself has objectRef as wll, to its channel field:

	//  The resource pointed by this ObjectReference must meet the Addressable contract
	//  with a reference to the Addressable duck type. If the resource does not meet this contract,
	//  it will be reflected in the Subscription's status.
	// +optional
	Channel *corev1.ObjectReference `json:"channel,omitempty"`

[matzew]

and as Roland said, seems to imply a channel only... not addressable.

so, perhaps best to get rid of it, and do what Sequence/Parallel do ?

any thoughts @vaikas-google, @Harwayne or @n3wscott ?

[rhuss]

Yes, I think skipping the ReplyStrategy level and just use an objectReference as value of the reply field (as suggested in #1936 (comment)) makes much sense.

[vaikas]

For consistency, it should probably be duck.Destination.

[n3wscott]

The history here was there were two levels of addressable. One was not persistent and assumed to be a filter or a mutator. One assumed control over the event and had no reply. This channel was required to be the persistent type but we did not have a way to force that. I bet this was updated to be a ref for the channel provisioner-to-CRD migration and no one noticed.

[lionelvillard]

/assign

[lionelvillard]

waiting for #1978 to either be merged or closed. It configures the subscription reconciler with a uriresolver that can be used to fix this bug

[lionelvillard]

Also Sequence and Parallel should adopt Destination