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.

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

Add a new Channel CRD to the eventing group #430

Merged
merged 17 commits into from
Sep 18, 2018

Conversation

grantr
Copy link
Contributor

@grantr grantr commented Sep 14, 2018

This is intended to replace the existing Channel in the channels group.
It doesn't include the array of subscription details yet.

This is intended to replace the existing Channel in the channels group.
It doesn't include the array of subscription details yet.
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 14, 2018

//TODO replace this with openapi defaults when
// https://github.com/kubernetes/features/issues/575 lands (scheduled for 1.13)
func (c *Channel) SetDefaults() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any reason to keep this empty defaults file and others like it? It pulls down our coverage numbers unless we write no-op tests.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to removing.

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to keep func (c *Channel) SetDefaults() { but you can delete func (fs *ChannelSpec) SetDefaults() {

The webhook validation code requires you implement Defaultable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you would like to change this, you will have to update GenericCRD interface and https://github.com/knative/pkg/blob/a3bc2db77a14d9ca6195172e81b4bf33e6190f85/webhook/webhook.go#L227


// A reference to the k8s Service backing this channel.
// +optional
Service *corev1.LocalObjectReference `json:"service,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a notion of capturing all provisioned resources in a generic way. Perhaps Provisioned []corev1.ObjectReference. Likely, this is overkill if those resources have OwnerReferences back to this Channel.

Service and VirtualService are too implementation specific and should be removed

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 these fields. We can add Provisioned later if owner references prove insufficient.

// ChannelConditionServiceable has status True when the service addressing the
// Channel exists.
// TODO should this be Sinkable?
ChannelConditionServiceable ChannelConditionType = "Serviceable"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a channel may not be fronted by a Service, "Serviceable" is no longer a good name. "Sinkable" (or whatever we end up calling that interface) is good. I'd also consider "Targetable" since that aligns with the .status.domainInternal value being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this condition since I can't think of a reason for it to be different from Ready.

These fields are too specific to the implementation of the Channel.
The Serviceable/Sinkable condition is effectively the same as Ready.
Allows ignoring timestamps when comparing Statuses for equality.
Shared by Channel and Source so it shouldn't be buried in the types for
either one.
Includes the Call and Result fields from Subscription.Spec so the
Subscription controller can copy those values directly.
Each Subscriber must have at least one of Call or Result.

Currently the validation returns immediately upon encountering an
empty Subscriber object due to a limitation in the knative/pkg
validation helpers. This is probably fine for now since this field is
not expected to be mutated by users.
@grantr
Copy link
Contributor Author

grantr commented Sep 17, 2018

Channel now has a Subscribers array, each of which contains a Call and Result field of the same type as that in SubscriptionSpec. Elements of the array are validated to contain at least one of the Call or Result fields.

ProvisionerReference was considered a top-level type at one point,
apparently.
The SetDefaults method is required to exist by the webhook
implementation, but we don't want it to negatively impact coverage
stats.
// +optional
// +patchMergeKey=type
// +patchStrategy=merge
Conditions []ChannelCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
Copy link
Contributor

Choose a reason for hiding this comment

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

you will want to update to the style of #434

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 get #434 in first, reviewing now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is in now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use duck Conditions.

Encapsulates the happy/dependent condition behavior.
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// Channel represents a named endpoint on which a Bus accepts event delivery and
// corresponds to the channels.channels.knative.dev CRD. The Bus handles
Copy link
Contributor

Choose a reason for hiding this comment

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

channels.channels.knative.dev -> channels.eventing.knative.dev?

// Check that ConfigurationStatus may have its conditions managed.
var _ duckv1alpha1.ConditionsAccessor = (*ChannelStatus)(nil)

// Check that Subscription implements the Conditions duck type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Subscription -> Channel

eventingv1alpha1.SchemeGroupVersion.WithKind("ClusterProvisioner"): &eventingv1alpha1.ClusterProvisioner{},
eventingv1alpha1.SchemeGroupVersion.WithKind("Subscription"): &eventingv1alpha1.Subscription{},
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch


//TODO replace this with openapi defaults when
// https://github.com/kubernetes/features/issues/575 lands (scheduled for 1.13)
func (c *Channel) SetDefaults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you would like to change this, you will have to update GenericCRD interface and https://github.com/knative/pkg/blob/a3bc2db77a14d9ca6195172e81b4bf33e6190f85/webhook/webhook.go#L227


import "testing"

// No-op test because method does nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

does nothing yet :D

// ObservedGeneration is the most recent generation observed for this Channel.
// It corresponds to the Channel's generation, which is updated on mutation by
// the API Server.
//TODO The above comment is only true once
Copy link
Contributor

Choose a reason for hiding this comment

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

malformed TODO. Should be:

// TODO: The above comment is only true once

// DomainInternal holds the top-level domain that will distribute traffic
// over the provided targets from inside the cluster. It generally has the
// form {channel}.{namespace}.svc.cluster.local
//TODO move this to a struct that can be embedded similar to ObjectMeta and
Copy link
Contributor

Choose a reason for hiding this comment

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

malformed TODO. See above

// +optional
// +patchMergeKey=type
// +patchStrategy=merge
Conditions []ChannelCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It is in now!

const (
// ChannelConditionReady has status True when the Channel is ready to accept
// traffic.
ChannelConditionReady ChannelConditionType = "Ready"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will turn into ChannelConditionReady = duck.ConditionReady


// ChannelConditionProvisioned has status True when the Channel's backing
// resources have been provisioned.
ChannelConditionProvisioned ChannelConditionType = "Provisioned"
Copy link
Contributor

Choose a reason for hiding this comment

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

duck.ConditionType

@knative-metrics-robot
Copy link

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 Delta
pkg/apis/eventing/v1alpha1/channel_defaults.go Do not exist 100.0%
pkg/apis/eventing/v1alpha1/channel_types.go Do not exist 100.0%
pkg/apis/eventing/v1alpha1/channel_validation.go Do not exist 100.0%

@vaikas
Copy link
Contributor

vaikas commented Sep 18, 2018

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, 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 [grantr,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-prow-robot knative-prow-robot merged commit 777657e into knative:master Sep 18, 2018
@grantr grantr deleted the new-channel-crds branch September 20, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants