Skip to content

Switch KRShaped interface to retrieve entire condition set #1307

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 3 commits into from
May 6, 2020
Merged

Switch KRShaped interface to retrieve entire condition set #1307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b75a2c
Switch interface to retrieve entire condition set
whaught May 6, 2020
0a3f8ea
fix method under test
whaught May 6, 2020
9abd332
type
whaught May 6, 2020
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
12 changes: 6 additions & 6 deletions apis/duck/v1/kresource_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ import (
"knative.dev/pkg/apis"
)

// KRShaped is an interface for retrieving the duck elements of an arbitraty resource.
// KRShaped is an interface for retrieving the duck elements of an arbitrary resource.
type KRShaped interface {
metav1.ObjectMetaAccessor

GetTypeMeta() *metav1.TypeMeta

GetStatus() *Status

GetTopLevelConditionType() apis.ConditionType
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 6, 2020

Choose a reason for hiding this comment

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

Here is a hitch in this plan: for things like alternate broker implementations, the reconciler for the broker gets to redefine what the ConditionSet for the resource is, and it ignores the type's conditions set, it also avoids all the Mark* methods. So it might not be true that this condition set is the one in play for the resource under reconciliation.

Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 6, 2020

Choose a reason for hiding this comment

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

cc: @grantr

Copy link
Copy Markdown
Contributor Author

@whaught whaught May 6, 2020

Choose a reason for hiding this comment

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

We we could either say alternate brokers are not KRShaped and need to manage things on their own or if they share the same top-level condition, define a set with only that.

Copy link
Copy Markdown
Contributor

@grantr grantr May 6, 2020

Choose a reason for hiding this comment

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

For the alternate broker in https://github.com/google/knative-gcp we have a separate API type that defines its own ConditionSet specific to the alternate implementation. There's a separate generated reconciler for that type that the alternate broker controller uses to reconcile. I'm not sure if this is the right pattern long-term.

Copy link
Copy Markdown
Contributor

@grantr grantr May 6, 2020

Choose a reason for hiding this comment

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

@n3wscott do you have an example of a single type that works with multiple ConditionSets? I don't know of any.

Copy link
Copy Markdown
Member

@mattmoor mattmoor May 6, 2020

Choose a reason for hiding this comment

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

Assuming "Top Level" means Ready/Succeeded, then generally conformance dictates that this should not vary. I think that the rest can vary at all levels of severity, assuming our semantics are adhered to.

Copy link
Copy Markdown
Contributor

@n3wscott n3wscott May 6, 2020

Choose a reason for hiding this comment

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

OK, just note we have an issue if we ever use any other method on the ConditionSet other than the toplevel method.

GetConditionSet() apis.ConditionSet
}

// Asserts KResource conformance with KRShaped
Expand Down Expand Up @@ -91,12 +91,12 @@ func (t *KResource) GetStatus() *Status {
return &t.Status
}

// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
func (t *KResource) GetTopLevelConditionType() apis.ConditionType {
// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
func (t *KResource) GetConditionSet() apis.ConditionSet {
// Note: KResources are unmarshalled from existing resources. This will only work properly for resources that
// have already been initialized to their type.
if cond := t.Status.GetCondition(apis.ConditionSucceeded); cond != nil {
return apis.ConditionSucceeded
return apis.NewBatchConditionSet()
}
return apis.ConditionReady
return apis.NewLivingConditionSet()
}
41 changes: 27 additions & 14 deletions apis/duck/v1/kresource_type_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,35 @@ import (
"knative.dev/pkg/apis"
)

func TestGetTopLevelCondition(t *testing.T) {
resource := KResource{}

condSet := apis.NewLivingConditionSet("Foo")
mgr := condSet.Manage(resource.GetStatus())
mgr.InitializeConditions()

if resource.GetTopLevelConditionType() != apis.ConditionReady {
t.Error("Expected Ready as happy condition for living condition set type")
func TestGetConditionSet(t *testing.T) {
testCases := []struct {
name string
condSet apis.ConditionSet
expectedTopLevelCondition apis.ConditionType
}{
{
name: "living set",
condSet: apis.NewLivingConditionSet(),
expectedTopLevelCondition: apis.ConditionReady,
},
{
name: "batch set",
condSet: apis.NewBatchConditionSet(),
expectedTopLevelCondition: apis.ConditionSucceeded,
},
}

condSet = apis.NewBatchConditionSet("Foo")
mgr = condSet.Manage(resource.GetStatus())
mgr.InitializeConditions()
for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
resource := KResource{}
test.condSet.Manage(resource.GetStatus()).InitializeConditions()

mgr := resource.GetConditionSet().Manage(resource.GetStatus())

if resource.GetTopLevelConditionType() != apis.ConditionSucceeded {
t.Error("Expected Succeeded as happy condition for living condition set type")
if mgr.GetTopLevelCondition().Type != test.expectedTopLevelCondition {
t.Errorf("wrong top-level condition got=%s want=%s",
mgr.GetTopLevelCondition().Type, test.expectedTopLevelCondition)
}
})
}
}
6 changes: 3 additions & 3 deletions apis/test/example/v1alpha1/fiz_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (f *ClusterFiz) GetStatus() *duckv1.Status {
return &f.Status.Status
}

// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
func (*ClusterFiz) GetTopLevelConditionType() apis.ConditionType {
return apis.ConditionReady
// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
func (*ClusterFiz) GetConditionSet() apis.ConditionSet {
return apis.NewLivingConditionSet()
}
6 changes: 3 additions & 3 deletions apis/test/example/v1alpha1/foo_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (f *Foo) GetStatus() *duckv1.Status {
return &f.Status.Status
}

// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
func (*Foo) GetTopLevelConditionType() apis.ConditionType {
return apis.ConditionSucceeded
// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
func (*Foo) GetConditionSet() apis.ConditionSet {
return apis.NewLivingConditionSet()
}
6 changes: 3 additions & 3 deletions apis/test/pub/v1alpha1/bar_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (b *Bar) GetStatus() *duckv1.Status {
return &b.Status.Status
}

// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
func (*Bar) GetTopLevelConditionType() apis.ConditionType {
return apis.ConditionReady
// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
func (*Bar) GetConditionSet() apis.ConditionSet {
return apis.NewLivingConditionSet()
}