Create new Subscription in pkg/apis/eventing as per the new spec. #421

vaikas · 2018-09-08T00:08:31Z

Fixes #

Proposed Changes

Add a new version of Subscription CRD as per the new Spec.

**** To reduce code churn, tests are incomplete Please only look at the subscription_types for now****

Release Note

scothis · 2018-09-08T01:16:26Z

👍 on the general direction.

One idea worth considering. Instead of using strings for these properties we could use ObjectReferences. The corresponding resolved values for Processor and To can be added to SubscriptionStatus, so that dispatchers still don't need to resolve in the data plane. A centralized subscription controller can take responsibility for synchronizing the resolutions. Webhook validation can reject invalid configurations.

Advantages:

higher level resources, like Flow, are easier to implement because they don't need to preform the resolution themselves
resolutions are automatically resynchronized over time. More important for non-controlled subscriptions
any component that needs to resolve these targets will need rbac get access to the target component. Centralizing the resolution also enables centralizing the privilege. Adding a new CRD means adding a single new Role/RoleBinding.
easy to add support for common resources that are unlikely to implement our targetable interface (like k8s core Service)
future: a ClusterChannel CRD was discussed as a structured way to pass events between namespaces. An ObjectReference makes it easier to support other types in the future. This should be very rare though.
future: access controls could be applied to the subscription

Disadvantages:

ObjectReferences are more complex (forces all creators to know the resource kind)
Subscription creators would be unable to create custom resolution strategies, unless we introduce a manual mode
there is a delay between when a target resource changes its domainInternal status and when the subscription is re-synced. The current Flow resource also suffers this delay, however, provisioners with foreknowledge are less able to trigger an immediate update.

n3wscott · 2018-09-10T14:59:09Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	// Processor is the processor service DNS name. Events
+	// from the From channel will be delivered here and replies are sent
+	// to To channel.
+	Processor string `json:"processor,omitempty"`


How about Call?

since kubernetes resource yaml describes the desired declarative state to be reconciled by a controller, I personally prefer to avoid using verbs that suggest the yaml is defining the imperative logic

From conversation in Slack, I'd like to see names which provide the following:

Clearly unambiguous about the sequence (from, processor, to seems ambiguous whether to or processor is last).

Work cleanly for all three cases (from, processor, to), (from, processor), (from, to).

Mostly, we seem to think from is clear. Here are some options for the other two:

processor: call, invoke, through, subscriber, apply

to: finally (collides with exception handling), then (may not be strong enough), result, out, continue (may collide with loops and not imply last item).

Here's an option where there's always a to/from pair at top-level. If both processor and channel are included in the to, the processor will always be invoked prior to sending its result to the channel.

from: x to: processor: foo channel: y

from: x to: processor: foo

from: x to: channel: y

The downside is that it adds a level of nesting in the config, but that might be worth the tradeoff since these resources would typically be generated from higher level resources and/or CLIs.

What's the upside of adding this extra level of nesting, since to: block will still have the same behaviour as if we kept it the top level?

n3wscott · 2018-09-10T14:59:22Z

pkg/apis/channels/v1alpha1/subscription_types.go

-	// Target service DNS name for replies returned by the subscriber.
-	ReplyTo string `json:"replyTo,omitempty"`
+	// To is the name of the channel to send transformed events
+	To string `json:"to,omitempty"`


How about Continue ?

vaikas · 2018-09-10T19:33:13Z

@scothis making them references is appealing for sure. I was left wondering how to actually then configure the underlying 'real channel' to wire things together. It's one thing to resolve the reference, but then once the wiring happens, how do you see that happening. Seems like once you resolve the referenced object, you'd need to modify that object to actually create the subscription. So, if a BetterChannel CRD was created and subscription resolved that it was the underlying channel, how would the SubscriptionController then know how to modify the BetterChannel resource?

scothis · 2018-09-10T19:57:29Z

@vaikas-google It may help to talk about the processor and to fields separately from the from field for this discussion. The processor and to fields are resolving from an ObjectRef to a domain name via the Targetable interface.

The from field is a bit more open ended since the Provisioner for the Channel is also the Provisioner for Subscription for that Channel. As an initial pass, I think we should force the ObjectRef Kind to channels.eventing.knativedev/v1alpha1 rejecting any other values. The Channel Provisioner is responsible for selecting which Subscriptions are for a Channel it provisioned and then provision the Subscription. For the stub bus this is by indexing the Subscription for the Channel in the dispatcher. For the Pub/Sub bus this is by creating a Subscription for the Topic in Cloud Pub/Sub and then receiving messages for that Subscription.

I don't think the SubscriptionController would ever modify the Channel resource, it would only be responsible for updating the SubcriptionStatus for the values it resolves. The Channel Provisioner is responsible for provisioning/reconciling the Subscription with the resolved properties.

vaikas · 2018-09-10T20:56:39Z

But the SubscriptionController needs to somehow communicate to the resolved channel that there's a new subscription?
I think it's odd that the ChannelController would be looking at a spec for an object it does not control.
I'd expect something like this (only From considered here):

Subscription is created: From is resolved to FromChannel
SubscriptionController modifies FromChannel.Spec to add this new subscription
ChannelController sees the FromChannel.Spec and does the thing it needs to do

Ideally (I think) Channel would have a subresource that the SubscriptionController would twiddle, but in lieue of that, we could make it so that the "contract" (given that we do refs, and we need to have some commonality there in the future) would be that a target of a ChannelRef resolve to an object that in it's spec has a Subscriptions List (or something like that), and the SubscriptionController then can just Read Modify Write the list by adding (or removing on deletion) the new (or deleted) subscriber.

And yes, rejecting anything other than channels.eventing.knative.dev/v1alpha1 for now is fine, just thinking how this could look like for superduperchannel that comes next and what that contract may look like.

Lastly, as per the document discussed, this will move to:
eventing.knative.dev/v1alpha1
I just made this WIP here to show only the minimal set of changes, once we agree on things, I'll move this to pkg/apis/eventing/v1alpha1/subscription_types

scothis · 2018-09-10T21:24:50Z

@vaikas-google Ah, I understand your proposal for Subscriptions now. The doc so far has been quiet about how Channel Provisioners discover Subscriptions.

Setting the resolved Subscriptions on the Channel resource should work so long as the subscriptions are fairly stable. It will create more work for the channel provisioner to tease apart which subscription was mutated/added/removed when the channel resource is updated. With the benefit of watching a single resource instead of two. I'll need to think about this a bit more.

Lastly, as per the document discussed, this will move to:
eventing.knative.dev/v1alpha1

Yes, I was merging the kind and apiVersion in my head. channels.eventing.knative.dev (future) vs channels.channels.knative.dev (today)

evankanderson

This only renames the fields, and does not update our samples or existing code.

My understanding is that this will be moved to the eventing.knative.dev package before submit, so as not to break current users.

evankanderson · 2018-09-10T23:35:08Z

pkg/apis/channels/v1alpha1/subscription_validation_test.go

+		name: "missing to",
+		c: &SubscriptionSpec{
+			From: "fromChannel",
+			To:   "toChannel",


Should this be "processor", or should "to" be left off? (This matches the next test case right now.)

evankanderson · 2018-09-10T23:41:12Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	// Processor is the processor service DNS name. Events
+	// from the From channel will be delivered here and replies are sent
+	// to To channel.
+	Processor string `json:"processor,omitempty"`


From conversation in Slack, I'd like to see names which provide the following:

Clearly unambiguous about the sequence (from, processor, to seems ambiguous whether to or processor is last).

Work cleanly for all three cases (from, processor, to), (from, processor), (from, to).

Mostly, we seem to think from is clear. Here are some options for the other two:

processor: call, invoke, through, subscriber, apply

to: finally (collides with exception handling), then (may not be strong enough), result, out, continue (may collide with loops and not imply last item).

vaikas · 2018-09-10T23:50:15Z

to->follower?

scothis

The validation logic from the previous commit will need to be updated now that these are ObjectRefs instead of Strings

scothis · 2018-09-11T21:05:49Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	// to these events for further processing.
+	//
+	// For example, this could be a reference to a Route resource
+	// or a Configuration resource.


s/Configuration/Service/

Configuration is not targetable.

scothis · 2018-09-11T21:07:43Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	//   - Kind
+	//   - APIVersion
+	//   - Name
+	From *corev1.ObjectReference `json:"from,omitempty"`

 	// Processor is the processor service DNS name. Events


no longer a DNS name

vaikas · 2018-09-11T21:12:48Z

@scothis yes, validation needs to change and things need to move to right location. Just trying to minimize churn and only focus on the API definition and semantics for now. Once we agree on that, I'll move this to eventing/pkg/apis/eventing/v1alpha1/ and do all the remaining work there.

scothis · 2018-09-12T19:31:46Z

pkg/apis/channels/v1alpha1/subscription_validation.go

+		return fe
+	}
+
+	if equality.Semantic.DeepEqual(ss.Processor, &corev1.ObjectReference{}) && equality.Semantic.DeepEqual(ss.To, &corev1.ObjectReference{}) {


could also cause issues if one is nil and the other is empty

scothis · 2018-09-12T19:36:15Z

pkg/apis/channels/v1alpha1/subscription_validation.go

@@ -49,7 +57,7 @@ func (current *Subscription) CheckImmutableFields(og apis.Immutable) *apis.Field
 		return nil
 	}

-	ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Subscriber", "Arguments")
+	ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Processor", "Arguments")


To should also allow mutation, only From should be immutable. It was an oversight that ReplyTo is immutable right now.

scothis · 2018-09-12T19:43:52Z

pkg/apis/channels/v1alpha1/subscription_validation_test.go

+	ProcessorName     = "processor"
+)
+
+func getValidFromRef() *corev1.ObjectReference {


should also test invalid From and To ObjectReferences:

non channel Kind and APIVersion

fields other than Kind, APIVersion and Name

Working on it... Just want to resolve the names...

evankanderson

(I'm not commenting on the naming beyond the 🚲 🏠 I did previously, but I'm not a big fan of the current from/processor/to names.)

evankanderson · 2018-09-12T20:32:21Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	// You can specify only the following fields of the ObjectReference:
+	//   - Kind
+	//   - APIVersion
+	//   - Name


What about UID?

I'm assuming that Namespace is specifically excluded?

What about UID?

Is there a good way to lookup a resource by UID?

I'm assuming that Namespace is specifically excluded?

Keeping subscriptions within a single namespace is a good idea until we understand fully understand the tenancy consequences of allowing them to cross namespaces.

Let's start with names (no UIDs), and a single namespace until we have more understanding on what cases this does not handel?

evankanderson · 2018-09-12T20:44:45Z

pkg/apis/channels/v1alpha1/subscription_validation.go

-		fe.Details = "the Subscription must reference a Channel"
+	if ss.From == nil || equality.Semantic.DeepEqual(ss.From, &corev1.ObjectReference{}) {
+		fe := apis.ErrMissingField("from")
+		fe.Details = "the Subscription must reference a from channel"


This should probably also ensure that the ss.From.Name and ss.From.Kind (at least) are non-empty.

(In general, it feels like there should be a function to validate an ObjectReference in the context of Subscription.)

le sigh :) Yes, there's bunch of more work to be done here :) Thanks for the comments, I just don't want to keep churning these things until we settle on the shape and the names. I'm going to add validation for now that it can only be a Channel for now as discussed in Slack yesterday, and later on we can relax.

evankanderson · 2018-09-12T20:49:23Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	//   - APIVersion
+	//   - Name
+	// +optional
+	Processor *corev1.ObjectReference `json:"processor,omitempty"`


Is the idea that you would need to create a k8s Service to be able to call something which is not on the k8s cluster (e.g. a VM)? Previously we had the idea that a targetable was either a DNS name or an ObjectReference.

Yeah, so, this is a bit interesting in a sense, that I'd expect that if you're hitting something that's outside the cluster, just a DNS entry is probably not going to cut it. I'd expect things like credentials to be involved on that, so as not to just be wide open to everything. I could hoist this into a higher level object to give us some future proofing for now.

Another option is to allow the reference for a targetable to also include a k8s Service. It doesn't implement the targetable interface, but we know how to shoehorn it into what we need.

@scothis when we talked about this wrt Targetable, thinking was that a service was not enough, that it would actually have to be a URI. So, I'm hoisting that up.

evankanderson · 2018-09-12T20:51:18Z

pkg/apis/channels/v1alpha1/subscription_types.go

+	//   - APIVersion
+	//   - Name
+	// +optional
+	To *corev1.ObjectReference `json:"to,omitempty"`


I can't comment on other parts of this CRD, so leaving the comment here:

Can we remove Arguments until we need them, and prefer to have Subscription-specific arguments rather than Channel-specific Arguments? (See also s/[]Arguments/runtime.RawExtension/.)

What conditions do we need? I think we probably want at least 2 Types: Ready and ReferencesResolved, where Ready is the canonical "happy state" type.

Do we need LastUpdateTime and LastTransitionTime? I'd be inclined to leave off that bookkeeping until we have evidence we need it.

Can we remove Arguments until we need them

Yes, we haven't come across a use case for argument/params with the current buses.

scothis · 2018-09-12T21:01:31Z

A subscription triggers sending events from a channel to a subscriber; the subscriber's replies (if any) are forwarded to a channel for subsequent processing.

spec:
  channel: <channel-ref>
  subscriber: <targetable-ref>
  replyStrategy:
    channel: <channel-ref>

The key elements of the SubscriptionSpec are the channel being subscribed to and the subscriber being targeted. Handling replies from the subscriber are managed by a strategy for which we have an two implementations:

forwarding to a hardwired channel
"/dev/null" where the reply is logged and tossed

As discussed in Slack we want the ability to define other strategies for mapping or dynamic resolution.

vaikas · 2018-09-12T21:08:17Z

@scothis this was the idea that you'd have Subscription coordinate with the Function what the return headers are and then dispatchers will dynamically route to different channels based on that?

markfisher · 2018-09-13T17:37:15Z

I'd like to make one last push for preferring nouns since the resource is describing desired state in a declarative way, not imperative logic. I had proposed "channel"->"subscriber" as the essence of what a Subscription describes, and the "replyStrategy" would describe how to handle a reply from the subscriber.

That said, if we are going to stick with "call" then I would prefer consistency for what is currently "result" (i.e. if one is a verb, they both should be). The latter could be "forward" or "reply" (I would probably choose "reply" since that is commonly used for describing this role in messaging systems - e.g. via "replyTo" headers - even if it's not immediately replying back to an original caller but instead continuing to flow through a sequence of subsequent channels/subscribers).

Also, if we are going to use a preposition, then "from" is actually not very clear, given that one subscribes to a channel (this was even more confusing when we also had "to" for the other direction).

scothis · 2018-09-13T17:39:23Z

This gives us an out for Channel-like things to be switched in-place of channels.

I want to be clear that I do not think we should support "channel-like things". The thinking was that we may have other kinds of channels in the future. The only concrete thought I have in this direction is a ClusterChannel to enable eventing between namespaces in a clearly defined way. Same behavior, but a separate CRD because that's how k8s works.

grantr · 2018-09-13T17:44:47Z

The only concrete thought I have in this direction is a ClusterChannel to enable eventing between namespaces in a clearly defined way. Same behavior, but a separate CRD because that's how k8s works.

I don't think @n3wscott is proposing that "channel-like things" don't implement the Channel contract; rather ClusterChannel is a perfect example of a "channel-like thing". I think we are in agreement. :)

grantr · 2018-09-13T20:12:12Z

pkg/apis/eventing/v1alpha1/subscription_types.go

+	"github.com/knative/pkg/webhook"
+	"k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
+	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"


Non-blocking nit: can this be renamed to metav1 for consistency?

vaikas · 2018-09-13T22:27:59Z

Ok, I think this is ready for a review. My most sincere apologies if I missed some comments in the flurry of discussions, here, doc and slack.

evankanderson

/approve

The naming seems good enough to me, just some minor nits. Glad to see that the only file with any real logic has 100% test coverage. Thank you!

evankanderson · 2018-09-13T22:43:28Z

pkg/apis/eventing/v1alpha1/subscription_defaults.go

+}
+
+func (ss *SubscriptionSpec) SetDefaults() {
+	// TODO anything?


Do we need this file if we don't default anything?

For consistency with other files, I'm leaving this in for now. If you feel strongly, I can remove.

evankanderson · 2018-09-13T22:44:50Z

pkg/apis/eventing/v1alpha1/subscription_types.go

+}
+
+// Callable specifies the reference to an object that's expected to
+// provide the resolved target of the action. Currently we inspect


Paragraph before "Currently"

evankanderson · 2018-09-13T22:46:25Z

pkg/apis/eventing/v1alpha1/subscription_types.go

+// This ensures that we can support external targets and for ease of use
+// we also allow for an URI to be specified.
+// There of course is also a requirement for the resolved Callable to
+// behave properly at the data plane level.


Do we want to document what this means?

I think we mean "Receive an event payload, and respond with one of: success and an optional response event, or failure. Delivery failures may be retries by the from Channel."

done. I felt like we should have a pointer to a spec and have a link to it here. We don't, so added what you proposed.

evankanderson · 2018-09-13T22:46:58Z

pkg/apis/eventing/v1alpha1/subscription_types.go

+// we can utilize a dynamic client instead of having to understand all
+// kinds of different types of objects. As long as they adhere to this
+// particular contract, they can be used as a Target.
+// This ensures that we can support external targets and for ease of use


Paragraph (blank line) here?

evankanderson · 2018-09-13T22:54:06Z

pkg/apis/eventing/v1alpha1/subscription_types.go

+type SubscriptionConditionType string
+
+const (
+	// SubscriptionReady is when the From,Channel and Result have been reconciled successfully.


"From,Channel and Result"?

"From, Call, and Result"?

Call seems to have both noun and verb definitions in computing (per Google), so I'm okay with these as declarative.

@evankanderson sure "call" can be used as a noun, but IMO "call" is going to be interpreted as a verb by anyone who uses this, especially when combined with "from", which is a preposition and the logical starting point among the components involved thereby setting an imperative context (also as I mentioned earlier, when creating a subscription you are actually subscribing TO a channel, not from).

I think all 3 should be nouns, and not ambiguous ones. I'm personally not even convinced the current "channel" and "subscriber" should change, since those are the most straightforwardly descriptive names for the 2 primary roles. Everything related to the handling of a subscriber's response is a side-effect, except in the (rare) "bridge" case, and for that I think it's best to view as a no-op subscriber since a channel should not be interpreted as the direct subscriber of another channel (even if the "bridge" is logical with no physical resource, it's enforcing the tinker-toy conceptual model, a.k.a. pipes-and-filters). I do see how "replyChannel" or something similar would be a better alternative to "replyTo" since the latter presents the same problem of an imperative context, and if we are going to support dynamic channel resolution in addition to hardwired literal values, then "replyStrategy" could be the top-level as @scothis proposed earlier (with the "mapping" example).

Sorry if this seems like bike-shedding. I do not view it that way, since these are not names used in an internal implementation. When defining the resources that users interact with, the names of those resources and their properties are among the most important things to get right.

evankanderson · 2018-09-13T22:56:33Z

pkg/apis/eventing/v1alpha1/subscription_validation.go

+}
+
+// We require always From
+// Also at least one of 'call' and 'result' must be defined (non-nill and non-empty)


(You mention "result", but we have no JSON element with that name.)

evankanderson · 2018-09-13T23:00:40Z

pkg/apis/eventing/v1alpha1/subscription_validation.go

+}
+
+func isValidCallable(c Callable) *apis.FieldError {
+	if c.TargetURI != nil && c.Target != nil && !equality.Semantic.DeepEqual(c.Target, &corev1.ObjectReference{}) {


Do you want to check for TargetURI != "" as well?

Do you need to check both c.Target != nil and !equality.Semantic.DeepEqual(c.Target, &corev1.ObjectReference{})? I'd expect that nil would not be equal to the other object reference.

Something in my readings led me to believe you needed to do this. I could totally be wrong about this however.

knative-prow-robot · 2018-09-14T00:24:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, vaikas-google

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson,vaikas-google]

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

knative-metrics-robot · 2018-09-14T04:45:16Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to run the coverage report again

File	Old Coverage	New Coverage
pkg/apis/eventing/v1alpha1/register.go	Do not exist	0.0%
pkg/apis/eventing/v1alpha1/subscription_defaults.go	Do not exist	0.0%
pkg/apis/eventing/v1alpha1/subscription_types.go	Do not exist	0.0%
pkg/apis/eventing/v1alpha1/subscription_validation.go	Do not exist	100.0%

vaikas · 2018-09-14T14:58:02Z

I'd like to unblock this work and suggest that we take the naming conversation to another thread. We still have tons of work to do before we can have a working end-to-end implementation and to unblock controller development, and provisioner implementation and so forth, I'd like to suggest that if the shape is right, we'll check this in and continue naming discussion and have an issue to revisit names before the release. Sound good?

…

On Fri, Sep 14, 2018 at 6:05 AM Mark Fisher ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/eventing/v1alpha1/subscription_types.go <#421 (comment)>: > + // This object must fulfill the Sinkable contract. + // + // TODO: Specify the required fields the target object must + // have in the status. + // You can specify only the following fields of the ObjectReference: + // - Kind + // - APIVersion + // - Name + // +optional + Target *corev1.ObjectReference `json:"target,omitempty"` +} + +type SubscriptionConditionType string + +const ( + // SubscriptionReady is when the From,Channel and Result have been reconciled successfully. @evankanderson <https://github.com/evankanderson> sure "call" *can* be used as a noun, but IMO "call" is going to be interpreted as a verb by anyone who uses this, especially when combined with "from", which is a preposition and the logical starting point among the components involved thereby setting an imperative context (also as I mentioned earlier, when creating a subscription you are actually subscribing TO a channel, not from). I think all 3 should be nouns, and not ambiguous ones. I'm personally not even convinced the current "channel" and "subscriber" should change, since those are the most straightforwardly descriptive names for the 2 primary roles. Everything related to the handling of a subscriber's response is a side-effect, except in the (rare) "bridge" case, and for that I think it's best to view as a no-op subscriber since a channel should not be interpreted as the direct subscriber of another channel (even if the "bridge" is logical with no physical resource, it's enforcing the tinker-toy conceptual model). I do see how "replyChannel" or something similar would be a better alternative to "replyTo" since the latter presents the same problem of an imperative context, and if we are going to support dynamic channel resolution in addition to hardwired literal values, then "replyStrategy" could be the top-level as @scothis <https://github.com/scothis> proposed earlier (with the "mapping" example). Sorry if this seems like bike-shedding. I do not view it that way, since these are not names used in an internal implementation. When defining the resources that users interact with, the names of those resources and their properties are among the most important things to get right. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#421 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKwedMPjlgNXqmUORoWJjV8Z1xrefFDNks5ua6mRgaJpZM4WfoH6> .

markfisher · 2018-09-14T15:23:22Z

@vaikas-google that is fine with me. In fact I think having a dedicated discussion about the names in a WG meeting with some proposals being presented with their rationale followed by discussion would be better than having that discussion intermingled with other comments in a PR. I'll make sure we don't forget to follow-up ;-)

markfisher · 2018-09-14T15:26:25Z

/lgtm

allow hock for override

Rename Subscription fields to: from,to,processor

313b88b

vaikas added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2018

knative-prow-robot requested review from grantr and inlined September 8, 2018 00:08

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2018

n3wscott reviewed Sep 10, 2018

View reviewed changes

evankanderson reviewed Sep 10, 2018

View reviewed changes

make things refs

758ae8c

scothis reviewed Sep 11, 2018

View reviewed changes

address pr feedback, remove left over cruft

dc67d24

Ville Aikas added 5 commits September 11, 2018 22:44

add the contracts we expect each object reference to fulfill

a20d789

use Sinkable contract for the To ref

7e2d059

update tests to refs

28be0a6

more tests

0c3fcd7

moar tests

785ddbc

scothis reviewed Sep 12, 2018

View reviewed changes

add test for Subscription object

973d77c

evankanderson reviewed Sep 12, 2018

View reviewed changes

processor -> call

b8a8e49

grantr reviewed Sep 13, 2018

View reviewed changes

add tests

c9fc3b7

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2018

address pr feedback

add1e88

vaikas removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2018

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2018

rename meta_v1 to metav1

6a6778a

evankanderson reviewed Sep 14, 2018

View reviewed changes

address PR comments

9237900

fbiville mentioned this pull request Sep 14, 2018

Move subscription management to new subscription subcommand projectriff/riff#775

Closed

vaikas removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2018

vaikas changed the title ~~[WIP] Rename Subscription fields to: from,to,processor~~ Create new Subscription in pkg/apis/eventing as per the new spec. Sep 14, 2018

knative-prow-robot assigned markfisher Sep 14, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2018

knative-prow-robot merged commit 10699f0 into knative:master Sep 14, 2018

vaikas deleted the channel-renames branch September 14, 2018 15:30

markfisher mentioned this pull request Sep 17, 2018

Move channel support from riff service|function create to riff subscription create subcommand projectriff/riff#771

Closed

n3wscott mentioned this pull request Sep 25, 2018

adding n3wscott to approvers. #465

Merged

markfisher mentioned this pull request Oct 2, 2018

Adding Knative Eventing Object Model targeting 0.1 release. #482

Merged

matzew added a commit to matzew/eventing that referenced this pull request Feb 10, 2020

Merge pull request knative#421 from matzew/patch_eventing_to_latest

9f34b41

allow hock for override

evankanderson mentioned this pull request Jul 20, 2021

Update (rewrite) eventing specs knative/specs#25

Merged

Create new Subscription in pkg/apis/eventing as per the new spec. #421

Create new Subscription in pkg/apis/eventing as per the new spec. #421

Conversation

vaikas commented Sep 8, 2018 • edited Loading

Proposed Changes

scothis commented Sep 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markfisher Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Sep 10, 2018

scothis commented Sep 10, 2018

vaikas commented Sep 10, 2018

scothis commented Sep 10, 2018

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Sep 10, 2018

scothis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scothis commented Sep 12, 2018

vaikas commented Sep 12, 2018

markfisher commented Sep 13, 2018 • edited Loading

scothis commented Sep 13, 2018

grantr commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

vaikas commented Sep 13, 2018

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markfisher Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 14, 2018

knative-metrics-robot commented Sep 14, 2018

vaikas commented Sep 14, 2018 via email

markfisher commented Sep 14, 2018

markfisher commented Sep 14, 2018

vaikas commented Sep 8, 2018 •

edited

Loading

markfisher Sep 11, 2018 •

edited

Loading

markfisher commented Sep 13, 2018 •

edited

Loading

grantr commented Sep 13, 2018 •

edited

Loading

markfisher Sep 14, 2018 •

edited

Loading