Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Sep 14, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
313b88b
Rename Subscription fields to: from,to,processor
Sep 8, 2018
758ae8c
make things refs
Sep 11, 2018
dc67d24
address pr feedback, remove left over cruft
Sep 11, 2018
a20d789
add the contracts we expect each object reference to fulfill
Sep 11, 2018
7e2d059
use Sinkable contract for the To ref
Sep 12, 2018
28be0a6
update tests to refs
Sep 12, 2018
0c3fcd7
more tests
Sep 12, 2018
785ddbc
moar tests
Sep 12, 2018
973d77c
add test for Subscription object
Sep 12, 2018
b8a8e49
processor -> call
Sep 12, 2018
38983f0
Revert changes to pkg/channels, move everything over to pkg/eventing
Sep 12, 2018
c9fc3b7
add tests
Sep 13, 2018
add1e88
address pr feedback
Sep 13, 2018
6a6778a
rename meta_v1 to metav1
Sep 13, 2018
9237900
address PR comments
Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions pkg/apis/channels/v1alpha1/subscription_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,14 @@ var _ apis.Immutable = (*Subscription)(nil)
var _ runtime.Object = (*Subscription)(nil)
var _ webhook.GenericCRD = (*ClusterBus)(nil)

// SubscriptionSpec specifies the Channel and Subscriber and the configuration
// arguments for the Subscription.
// SubscriptionSpec specifies the Channel for incoming events, a handler and the Channel
// for outgoing messages.
// from --[transform]--> to
// Note that the following are valid configurations also:
// Sink, no outgoing events:
// from -- transform
// no-op function (identity transformation):
// from --> to
type SubscriptionSpec struct {
// TODO: Generation does not work correctly with CRD. They are scrubbed
// by the APIserver (https://github.com/kubernetes/kubernetes/issues/58778)
Expand All @@ -57,14 +63,17 @@ type SubscriptionSpec struct {
// +optional
Generation int64 `json:"generation,omitempty"`

// Channel is the name of the channel to subscribe to.
Channel string `json:"channel"`
// From is the name of the channel to subscribe to for receiving events
// to be transformed.
From string `json:"from"`

// Subscriber is the name of the subscriber service DNS name.
Subscriber string `json:"subscriber"`
// Processor is the processor service DNS name. Events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer a DNS name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// from the From channel will be delivered here and replies are sent
// to To channel.
Processor string `json:"processor,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calls ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Clearly unambiguous about the sequence (from, processor, to seems ambiguous whether to or processor is last).
  2. 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).

Copy link
Contributor

@markfisher markfisher Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Continue ?


// Arguments is a list of configuration arguments for the Subscription. The
// Arguments for a channel must contain values for each of the Parameters
Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/channels/v1alpha1/subscription_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ func (s *Subscription) Validate() *apis.FieldError {
}

func (ss *SubscriptionSpec) Validate() *apis.FieldError {
if ss.Channel == "" {
fe := apis.ErrMissingField("channel")
fe.Details = "the Subscription must reference a Channel"
if ss.From == "" {
fe := apis.ErrMissingField("from")
fe.Details = "the Subscription must reference a from channel"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return fe
}
if ss.Subscriber == "" {
fe := apis.ErrMissingField("subscriber")
fe.Details = "the Subscription must reference a Subscriber"
if ss.To == "" && ss.Processor == "" {
fe := apis.ErrMissingField("to", "processor")
fe.Details = "the Subscription must reference a to channel or a processor"
return fe
}
return nil
Expand All @@ -49,7 +49,7 @@ func (current *Subscription) CheckImmutableFields(og apis.Immutable) *apis.Field
return nil
}

ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Subscriber", "Arguments")
ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Processor", "Arguments")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if diff := cmp.Diff(original.Spec, current.Spec, ignoreArguments); diff != "" {
return &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Expand Down
64 changes: 39 additions & 25 deletions pkg/apis/channels/v1alpha1/subscription_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,34 +30,48 @@ func TestSubscriptionSpecValidation(t *testing.T) {
}{{
name: "valid",
c: &SubscriptionSpec{
Channel: "bar",
Subscriber: "foo",
From: "fromChannel",
Processor: "processor",
},
want: nil,
}, {
name: "valid with arguments",
c: &SubscriptionSpec{
Channel: "bar",
Subscriber: "foo",
Arguments: &[]Argument{{Name: "foo", Value: "bar"}},
From: "fromChannel",
Processor: "processor",
Arguments: &[]Argument{{Name: "foo", Value: "bar"}},
},
want: nil,
}, {
name: "missing subscriber",
name: "missing processor and to",
c: &SubscriptionSpec{
Channel: "foo",
From: "fromChannel",
},
want: func() *apis.FieldError {
fe := apis.ErrMissingField("subscriber")
fe.Details = "the Subscription must reference a Subscriber"
fe := apis.ErrMissingField("to", "processor")
fe.Details = "the Subscription must reference a to channel or a processor"
return fe
}(),
}, {
name: "missing to",
c: &SubscriptionSpec{
From: "fromChannel",
To: "toChannel",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
want: nil,
}, {
name: "missing processor",
c: &SubscriptionSpec{
From: "fromChannel",
To: "toChannel",
},
want: nil,
}, {
name: "empty",
c: &SubscriptionSpec{},
want: func() *apis.FieldError {
fe := apis.ErrMissingField("channel")
fe.Details = "the Subscription must reference a Channel"
fe := apis.ErrMissingField("from")
fe.Details = "the Subscription must reference a from channel"
return fe
}(),
}}
Expand All @@ -66,7 +80,7 @@ func TestSubscriptionSpecValidation(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
got := test.c.Validate()
if diff := cmp.Diff(test.want, got); diff != "" {
t.Errorf("validateChannel (-want, +got) = %v", diff)
t.Errorf("validateFrom (-want, +got) = %v", diff)
}
})
}
Expand All @@ -82,48 +96,48 @@ func TestSubscriptionImmutable(t *testing.T) {
name: "valid",
c: &Subscription{
Spec: SubscriptionSpec{
Channel: "foo",
From: "foo",
},
},
og: &Subscription{
Spec: SubscriptionSpec{
Channel: "foo",
From: "foo",
},
},
want: nil,
}, {
name: "valid, new subscriber",
name: "valid, new processor",
c: &Subscription{
Spec: SubscriptionSpec{
Channel: "foo",
Subscriber: "bar",
From: "foo",
Processor: "newProcessor",
},
},
og: &Subscription{
Spec: SubscriptionSpec{
Channel: "foo",
Subscriber: "baz",
From: "foo",
Processor: "processor",
},
},
want: nil,
}, {
name: "channel changed",
name: "from changed",
c: &Subscription{
Spec: SubscriptionSpec{
Channel: "foo",
From: "fromChannel",
},
},
og: &Subscription{
Spec: SubscriptionSpec{
Channel: "bar",
From: "newFromChannel",
},
},
want: &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Paths: []string{"spec"},
Details: `{v1alpha1.SubscriptionSpec}.Channel:
-: "bar"
+: "foo"
Details: `{v1alpha1.SubscriptionSpec}.From:
-: "newFromChannel"
+: "fromChannel"
`,
},
}}
Expand Down