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

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
45 changes: 38 additions & 7 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 @@ -63,17 +63,48 @@ type SubscriptionSpec struct {
// +optional
Generation int64 `json:"generation,omitempty"`

// From is the name of the channel to subscribe to for receiving events
// to be transformed.
From string `json:"from"`
// Reference to an object that will be used to create the subscription
// for receiving events. The object must have spec.subscriptions
// list which will then be modified accordingly.
//
// You can specify only the following fields of the ObjectReference:
// - Kind
// - APIVersion
// - Name
From *corev1.ObjectReference `json:"from,omitempty"`

// 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"`

// To is the name of the channel to send transformed events
To string `json:"to,omitempty"`
// Reference to an object that will be used to deliver events for
// (optional) processing before sending them to To for further
// if specified for additional Subscriptions to then subscribe
// to these events for further processing.
//
// For example, this could be a reference to a Route resource
// or a Configuration resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Configuration/Service/

Configuration is not targetable.

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

// 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
Processor *corev1.ObjectReference `json:"processor,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// To is the (optional) resolved channel where (optionally) processed
// events get sent.
//
// This has to be a channel
//
// 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
Copy link
Member

Choose a reason for hiding this comment

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

What about UID?

I'm assuming that Namespace is specifically excluded?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// +optional
To *corev1.ObjectReference `json:"to,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. 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/.)
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove Arguments until we need them

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.


// 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