Skip to content

Upgrading knative.eventing resources to use knative.pkg.Conditions #434

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

Merged
knative-prow-robot merged 2 commits into from
Sep 17, 2018
Merged

Upgrading knative.eventing resources to use knative.pkg.Conditions #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e07448
Upgrading knative.eventing resoureces to use knative.pkg.Conditions
Sep 17, 2018
17b4384
Merge with master.
Sep 17, 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
11 changes: 9 additions & 2 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ required = [

[[constraint]]
name = "github.com/knative/pkg"
# HEAD as of 2018-09-12
revision = "8fc80deb200e7853795e6baf4029f406ca9534b8"
# HEAD as of 2018-09-17
revision = "67830c7a64431edc38ee19628dc7116b0c6d99b5"

[[constraint]]
name = "github.com/knative/serving"
Expand Down
51 changes: 18 additions & 33 deletions pkg/apis/eventing/v1alpha1/cluster_provisioner_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ limitations under the License.
package v1alpha1

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"

"encoding/json"
"github.com/knative/pkg/apis"
duck "github.com/knative/pkg/apis/duck/v1alpha1"
"github.com/knative/pkg/webhook"
)

Expand Down Expand Up @@ -52,6 +52,9 @@ var _ apis.Defaultable = (*ClusterProvisioner)(nil)
var _ runtime.Object = (*ClusterProvisioner)(nil)
var _ webhook.GenericCRD = (*ClusterProvisioner)(nil)

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

// ClusterProvisionerSpec is the spec for a ClusterProvisioner resource.
type ClusterProvisionerSpec struct {
// TODO: Generation does not work correctly with CRD. They are scrubbed
Expand All @@ -67,42 +70,12 @@ type ClusterProvisionerSpec struct {
Reconciles metav1.GroupKind `json:"reconciles"`
}

type ClusterProvisionerConditionType string

const (
// ClusterProvisionerConditionReady specifies that the resource is ready.
ClusterProvisionerConditionReady ClusterProvisionerConditionType = "Ready"
)

// ClusterProvisionerConditionStatus describes the state of this resource at a point in time.
type ClusterProvisionerConditionStatus struct {
// Type of condition.
// +required
Type ClusterProvisionerConditionType `json:"type"`

// Status of the condition, one of True, False, Unknown.
// +required
Status corev1.ConditionStatus `json:"status"`

// LastTransitionTime is the last time the condition transitioned from one status to another.
// We use VolatileTime in place of metav1.Time to exclude this from creating equality.Semantic
// differences (all other things held constant).
// +optional
LastTransitionTime apis.VolatileTime `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`

// The reason for the condition's last transition.
// +optional
Reason string `json:"reason,omitempty"`

// A human readable message indicating details about the transition.
// +optional
Message string `json:"message,omitempty"`
}
var cProvCondSet = duck.NewLivingConditionSet()
Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

It wasn't immediately clear to me what makes this ConditionSet Living, or that this method automatically adds a Ready condition. Can you add a comment here explaining that?

An alternate solution is to eliminate the Living and Batch methods and expect the first argument to always be the happy condition:

var subCondSet = duck.NewConditionSet(duck.ConditionReady)

IMO this is more self-documenting.

Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

/cc @mattmoor who I'm told is responsible for the Living/Batch naming 😁

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Sep 18, 2018

Choose a reason for hiding this comment

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

cc @evankanderson @mattmoor on this comment.

Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

Is cProvCondSet ever used? I can't find any references to it.

Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

@n3wscott tells me uses of this variable will arrive in a future PR.


// ClusterProvisionerStatus is the status for a ClusterProvisioner resource
type ClusterProvisionerStatus struct {
// Conditions holds the state of a cluster provisioner at a point in time.
Conditions []ClusterProvisionerConditionStatus `json:"conditions,omitempty"`
Conditions duck.Conditions `json:"conditions,omitempty"`
Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

Why doesn't this field have the patch tags that SubscriptionStatus.Conditions has?


// ObservedGeneration is the 'Generation' of the ClusterProvisioner that
// was last reconciled by the controller.
Expand All @@ -124,3 +97,15 @@ type ClusterProvisionerList struct {

Items []ClusterProvisioner `json:"items"`
}

// GetConditions returns the Conditions array. This enables generic handling of
// conditions by implementing the duck.Conditions interface.
func (ps *ClusterProvisionerStatus) GetConditions() duck.Conditions {
return ps.Conditions
}

// SetConditions sets the Conditions array. This enables generic handling of
// conditions by implementing the duck.Conditions interface.
func (ps *ClusterProvisionerStatus) SetConditions(conditions duck.Conditions) {
ps.Conditions = conditions
}
47 changes: 20 additions & 27 deletions pkg/apis/eventing/v1alpha1/subscription_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import (
"encoding/json"

"github.com/knative/pkg/apis"
duck "github.com/knative/pkg/apis/duck/v1alpha1"
"github.com/knative/pkg/webhook"
"k8s.io/api/core/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -48,6 +48,9 @@ var _ apis.Immutable = (*Subscription)(nil)
var _ runtime.Object = (*Subscription)(nil)
var _ webhook.GenericCRD = (*Subscription)(nil)

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

// SubscriptionSpec specifies the Channel for incoming events, a Call target for
// processing those events and where to put the result of the processing. Only
// From (where the events are coming from) is always required. You can optionally
Expand Down Expand Up @@ -160,40 +163,18 @@ type ResultStrategy struct {
Target *corev1.ObjectReference `json:"target,omitempty"`
}

type SubscriptionConditionType string

const (
// SubscriptionReady is when the From,Call and Result have been reconciled successfully.
SubscriptionReady SubscriptionConditionType = "Ready"
)

// SubscriptionCondition describes the state of a subscription at a point in time.
type SubscriptionCondition struct {
// Type of subscription condition.
Type SubscriptionConditionType `json:"type"`
// Status of the condition, one of True, False, Unknown.
Status v1.ConditionStatus `json:"status"`
// The reason for the condition's last transition.
Reason string `json:"reason,omitempty"`
// A human readable message indicating details about the transition.
Message string `json:"message,omitempty"`
}
var subCondSet = duck.NewLivingConditionSet()

// SubscriptionStatus (computed) for a subscription
type SubscriptionStatus struct {
// Represents the latest available observations of a subscription's current state.
// +patchMergeKey=type
// +patchStrategy=merge
Conditions []SubscriptionCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
Conditions duck.Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

Does the patchStrategy tag work for this type?

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Sep 18, 2018

Choose a reason for hiding this comment

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

It looks like it does! Because Conditions is a slice

}

func (ss *SubscriptionStatus) GetCondition(t SubscriptionConditionType) *SubscriptionCondition {
for _, cond := range ss.Conditions {
if cond.Type == t {
return &cond
}
}
return nil
func (ss *SubscriptionStatus) GetCondition(t duck.ConditionType) *duck.Condition {
Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

Why isn't this method defined for ClusterProvisionerStatus?

Copy link
Copy Markdown
Contributor

@grantr grantr Sep 17, 2018

Choose a reason for hiding this comment

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

@n3wscott tells me this method or something like it will arrive in a future PR.

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Sep 18, 2018

Choose a reason for hiding this comment

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

Added it.

return subCondSet.Manage(ss).GetCondition(t)
}

func (s *Subscription) GetSpecJSON() ([]byte, error) {
Expand All @@ -208,3 +189,15 @@ type SubscriptionList struct {
metav1.ListMeta `json:"metadata"`
Items []Subscription `json:"items"`
}

// GetConditions returns the Conditions array. This enables generic handling of
// conditions by implementing the duck.Conditions interface.
func (ss *SubscriptionStatus) GetConditions() duck.Conditions {
return ss.Conditions
}

// SetConditions sets the Conditions array. This enables generic handling of
// conditions by implementing the duck.Conditions interface.
func (ss *SubscriptionStatus) SetConditions(conditions duck.Conditions) {
ss.Conditions = conditions
}
42 changes: 6 additions & 36 deletions pkg/apis/eventing/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions vendor/github.com/knative/pkg/apis/duck/doc.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions vendor/github.com/knative/pkg/apis/duck/register.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading