Skip to content

Pull in support for non-terminal conditions #2436

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 1 commit into from
Nov 9, 2018
Merged

Pull in support for non-terminal conditions #2436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 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 @@ -23,8 +23,8 @@ required = [

[[override]]
name = "github.com/knative/pkg"
# HEAD as of 2018-11-06
revision = "088e3f7faf9d7ed0122937f66289cc050d0a78b0"
# HEAD as of 2018-11-08
revision = "4b704fa7948ad9ae8ec90d1cd5b4a34516b252ea"

[[override]]
name = "go.uber.org/zap"
Expand Down
5 changes: 3 additions & 2 deletions pkg/apis/serving/v1alpha1/revision_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,9 @@ func TestGetSetCondition(t *testing.T) {
}

rc := &duckv1alpha1.Condition{
Type: RevisionConditionBuildSucceeded,
Status: corev1.ConditionTrue,
Type: RevisionConditionBuildSucceeded,
Status: corev1.ConditionTrue,
Severity: duckv1alpha1.ConditionSeverityError,
}

rs.PropagateBuildStatus(duckv1alpha1.KResourceStatus{
Expand Down
30 changes: 18 additions & 12 deletions pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,17 @@ func TestReconcile(t *testing.T) {
},
},
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ClusterIngressConditionLoadBalancerReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionLoadBalancerReady,
Status: corev1.ConditionTrue,
Severity: "Error",
Copy link
Copy Markdown
Contributor

@lichuqiang lichuqiang Nov 9, 2018

Choose a reason for hiding this comment

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

To avoid modifying things everywhere, can we have a Conditions wrapper in test, that default the Severity to "Error" if not specified?

Copy link
Copy Markdown
Member Author

@mattmoor mattmoor Nov 9, 2018

Choose a reason for hiding this comment

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

I debated adding Severity to the set of things ignored in the base table test (currently: LastTransitionTime), but we still need to provide this to our inputs or we'll get hard to decode "unexpected update" errors that seemingly match what we passed in.

It is clear that we need to refactor these table tests so there is less churn, but I don't want to block knative/pkg updates waiting on that. I'll play with this tomorrow.

Copy link
Copy Markdown
Contributor

@lichuqiang lichuqiang Nov 9, 2018

Choose a reason for hiding this comment

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

but I don't want to block knative/pkg updates waiting on that. I'll play with this tomorrow.

I'm fine with this

}, {
Type: v1alpha1.ClusterIngressConditionNetworkConfigured,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionNetworkConfigured,
Status: corev1.ConditionTrue,
Severity: "Error",
}, {
Type: v1alpha1.ClusterIngressConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand Down Expand Up @@ -137,14 +140,17 @@ func TestReconcile(t *testing.T) {
},
},
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ClusterIngressConditionLoadBalancerReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionLoadBalancerReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}, {
Type: v1alpha1.ClusterIngressConditionNetworkConfigured,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionNetworkConfigured,
Status: corev1.ConditionTrue,
Severity: "Error",
}, {
Type: v1alpha1.ClusterIngressConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ClusterIngressConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand Down
120 changes: 70 additions & 50 deletions pkg/reconciler/v1alpha1/configuration/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "no-revisions-yet-01234",
ObservedGeneration: 1234,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
}),
}},
Expand All @@ -99,10 +100,11 @@ func TestReconcile(t *testing.T) {
Object: setConcurrencyModel(cfgWithStatus("validation-failure", "foo", 1234,
v1alpha1.ConfigurationStatus{
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "invalid value \"Bogus\": spec.concurrencyModel".`,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "invalid value \"Bogus\": spec.concurrencyModel".`,
Severity: "Error",
}},
}), "Bogus"),
}},
Expand All @@ -127,8 +129,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "need-rev-and-build-99998",
ObservedGeneration: 99998,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
},
),
Expand All @@ -153,8 +156,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "need-rev-and-build-99998",
ObservedGeneration: 99998,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
},
),
Expand All @@ -172,8 +176,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "matching-revision-not-done-05432",
ObservedGeneration: 5432,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
},
),
Expand All @@ -192,8 +197,9 @@ func TestReconcile(t *testing.T) {
LatestReadyRevisionName: "matching-revision-done-05555",
ObservedGeneration: 5555,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand All @@ -208,8 +214,9 @@ func TestReconcile(t *testing.T) {
LatestReadyRevisionName: "matching-revision-done-idempotent-05566",
ObservedGeneration: 5566,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand All @@ -228,10 +235,11 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "matching-revision-failed-05555",
ObservedGeneration: 5555,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision "matching-revision-failed-05555" failed with message: "It's the end of the world as we know it".`,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision "matching-revision-failed-05555" failed with message: "It's the end of the world as we know it".`,
Severity: "Error",
}},
},
),
Expand All @@ -244,8 +252,9 @@ func TestReconcile(t *testing.T) {
makeRevStatus(resources.MakeRevision(cfg("bad-condition", "foo", 5555), nil),
v1alpha1.RevisionStatus{
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.RevisionConditionReady,
Status: "Bad",
Type: v1alpha1.RevisionConditionReady,
Status: "Bad",
Severity: "Error",
}},
},
),
Expand All @@ -257,8 +266,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "bad-condition-05555",
ObservedGeneration: 5555,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
},
),
Expand All @@ -282,10 +292,11 @@ func TestReconcile(t *testing.T) {
Object: cfgWithBuildAndStatus("create-build-failure", "foo", 99998, &buildSpec,
v1alpha1.ConfigurationStatus{
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "inducing failure for create builds".`,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "inducing failure for create builds".`,
Severity: "Error",
}},
},
),
Expand All @@ -308,10 +319,11 @@ func TestReconcile(t *testing.T) {
Object: cfgWithStatus("create-revision-failure", "foo", 99998,
v1alpha1.ConfigurationStatus{
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "inducing failure for create revisions".`,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision creation failed with message: "inducing failure for create revisions".`,
Severity: "Error",
}},
},
),
Expand All @@ -336,8 +348,9 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "update-config-failure-01234",
ObservedGeneration: 1234,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionUnknown,
Severity: "Error",
}},
},
),
Expand All @@ -351,10 +364,11 @@ func TestReconcile(t *testing.T) {
LatestCreatedRevisionName: "revision-recovers-01337",
LatestReadyRevisionName: "revision-recovers-01337",
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision "revision-recovers-01337" failed with message: "Weebles wobble, but they don't fall down".`,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionFalse,
Reason: "RevisionFailed",
Message: `Revision "revision-recovers-01337" failed with message: "Weebles wobble, but they don't fall down".`,
Severity: "Error",
}},
},
),
Expand All @@ -367,8 +381,9 @@ func TestReconcile(t *testing.T) {
LatestReadyRevisionName: "revision-recovers-01337",
ObservedGeneration: 1337,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand Down Expand Up @@ -404,8 +419,9 @@ func TestGCReconcile(t *testing.T) {
LatestReadyRevisionName: "old-revision-05556",
ObservedGeneration: 5556,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand All @@ -424,8 +440,9 @@ func TestGCReconcile(t *testing.T) {
LatestReadyRevisionName: "old-revision-05556",
ObservedGeneration: 5556,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand All @@ -445,8 +462,9 @@ func TestGCReconcile(t *testing.T) {
LatestReadyRevisionName: "old-revision-05556",
ObservedGeneration: 5556,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand All @@ -469,8 +487,9 @@ func TestGCReconcile(t *testing.T) {
LatestReadyRevisionName: "old-revision-05556",
ObservedGeneration: 5556,
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.ConfigurationConditionReady,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
},
),
Expand Down Expand Up @@ -564,8 +583,9 @@ func makeRevReady(t *testing.T, rev *v1alpha1.Revision) *v1alpha1.Revision {
rev.Status.MarkActive()
rev.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{
Conditions: []duckv1alpha1.Condition{{
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionTrue,
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionTrue,
Severity: "Error",
}},
})
if !rev.Status.IsReady() {
Expand Down
3 changes: 3 additions & 0 deletions pkg/reconciler/v1alpha1/revision/revision_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ func TestResolutionFailed(t *testing.T) {
Reason: "ContainerMissing",
Message: errorMessage,
LastTransitionTime: got.LastTransitionTime,
Severity: "Error",
}
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Unexpected revision conditions diff (-want +got): %v", diff)
Expand Down Expand Up @@ -463,6 +464,7 @@ func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) {
Status: corev1.ConditionUnknown,
Reason: "Deploying",
LastTransitionTime: got.LastTransitionTime,
Severity: "Error",
}
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Unexpected revision conditions diff (-want +got): %v", diff)
Expand All @@ -489,6 +491,7 @@ func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) {
Type: ct,
Status: corev1.ConditionTrue,
LastTransitionTime: got.LastTransitionTime,
Severity: "Error",
}
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Unexpected revision conditions diff (-want +got): %v", diff)
Expand Down
Loading