-
Notifications
You must be signed in to change notification settings - Fork 600
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
Upgrading knative.eventing resources to use knative.pkg.Conditions #434
Upgrading knative.eventing resources to use knative.pkg.Conditions #434
Conversation
/assign @grantr |
1a1a711
to
17b4384
Compare
} | ||
|
||
// Populate implements duck.Populatable | ||
func (t *KResource) Populate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sure that the object can be converted to JSON and back. note it is vendor code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also my birthday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, hence the LGTM/Approve :) But, just curious why this is not say in test code or somewhere else.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: n3wscott, 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:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to never have to write conditions methods again!
// +optional | ||
Message string `json:"message,omitempty"` | ||
} | ||
var cProvCondSet = duck.NewLivingConditionSet() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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.
} | ||
} | ||
return nil | ||
func (ss *SubscriptionStatus) GetCondition(t duck.ConditionType) *duck.Condition { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it.
// +optional | ||
Message string `json:"message,omitempty"` | ||
} | ||
var cProvCondSet = duck.NewLivingConditionSet() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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?
|
||
// 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"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Proposed Changes