Don't special-case the Build condition.

To allow treating `BuildSucceeded` as a terminal condition when we adjust the model as outlined in knative/pkg#161, we need to pass it to the `ConditionSet` constructor. This flushes out the downstream changes of that.
mattmoor · Nov 3, 2018 · 562ff93 · 562ff93
1 parent 2865b4c
commit 562ff93
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 233 deletions.
diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go
@@ -200,6 +200,7 @@ var revCondSet = duckv1alpha1.NewLivingConditionSet(
 	RevisionConditionResourcesAvailable,
 	RevisionConditionContainerHealthy,
 	RevisionConditionActive,
+	RevisionConditionBuildSucceeded,
 )
 
 var buildCondSet = duckv1alpha1.NewBatchConditionSet()
@@ -297,13 +298,6 @@ func (rs *RevisionStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alph
 
 func (rs *RevisionStatus) InitializeConditions() {
 	revCondSet.Manage(rs).InitializeConditions()
-
-	// We don't include BuildSucceeded here because it could confuse users if
-	// no `buildName` was specified.
-}
-
-func (rs *RevisionStatus) InitializeBuildCondition() {
-	revCondSet.Manage(rs).InitializeCondition(RevisionConditionBuildSucceeded)
 }
 
 func (rs *RevisionStatus) PropagateBuildStatus(bs duckv1alpha1.KResourceStatus) {
@@ -374,8 +368,8 @@ func (rs *RevisionStatus) SetConditions(conditions duckv1alpha1.Conditions) {
 const (
 	AnnotationParseErrorTypeMissing = "Missing"
 	AnnotationParseErrorTypeInvalid = "Invalid"
-	LabelParserErrorTypeMissing = "Missing"
-	LabelParserErrorTypeInvalid = "Invalid"
+	LabelParserErrorTypeMissing     = "Missing"
+	LabelParserErrorTypeInvalid     = "Invalid"
 )
 
 // +k8s:deepcopy-gen=false

diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go
@@ -300,7 +300,6 @@ func TestGetSetCondition(t *testing.T) {
 func TestTypicalFlowWithBuild(t *testing.T) {
 	r := &Revision{}
 	r.Status.InitializeConditions()
-	r.Status.InitializeBuildCondition()
 	checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t)
 	checkConditionOngoingRevision(r.Status, RevisionConditionResourcesAvailable, t)
 	checkConditionOngoingRevision(r.Status, RevisionConditionContainerHealthy, t)
@@ -394,19 +393,11 @@ func TestTypicalFlowWithBuild(t *testing.T) {
 	checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t)
 	checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t)
 	checkConditionSucceededRevision(r.Status, RevisionConditionReady, t)
-
-	// Or this.
-	r.Status.InitializeBuildCondition()
-	checkConditionSucceededRevision(r.Status, RevisionConditionBuildSucceeded, t)
-	checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t)
-	checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t)
-	checkConditionSucceededRevision(r.Status, RevisionConditionReady, t)
 }
 
 func TestTypicalFlowWithBuildFailure(t *testing.T) {
 	r := &Revision{}
 	r.Status.InitializeConditions()
-	r.Status.InitializeBuildCondition()
 	checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t)
 	checkConditionOngoingRevision(r.Status, RevisionConditionResourcesAvailable, t)
 	checkConditionOngoingRevision(r.Status, RevisionConditionContainerHealthy, t)
@@ -515,6 +506,13 @@ func TestTypicalFlowWithSuspendResume(t *testing.T) {
 	r.Status.MarkActive()
 	r.Status.MarkContainerHealthy()
 	r.Status.MarkResourcesAvailable()
+	r.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{
+		Conditions: []duckv1alpha1.Condition{{
+			Type:   duckv1alpha1.ConditionSucceeded,
+			Status: corev1.ConditionTrue,
+			Reason: "NoBuild",
+		}},
+	})
 	checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t)
 	checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t)
 	checkConditionSucceededRevision(r.Status, RevisionConditionActive, t)
@@ -769,7 +767,7 @@ func TestRevisionAnnotations(t *testing.T) {
 			rev := Revision{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: tc.annotations,
-					Labels: tc.labels,
+					Labels:      tc.labels,
 				},
 			}
 

diff --git a/pkg/reconciler/v1alpha1/configuration/configuration_test.go b/pkg/reconciler/v1alpha1/configuration/configuration_test.go
@@ -531,6 +531,12 @@ func makeRevReady(t *testing.T, rev *v1alpha1.Revision) *v1alpha1.Revision {
 	rev.Status.MarkContainerHealthy()
 	rev.Status.MarkResourcesAvailable()
 	rev.Status.MarkActive()
+	rev.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{
+		Conditions: []duckv1alpha1.Condition{{
+			Type:   duckv1alpha1.ConditionSucceeded,
+			Status: corev1.ConditionTrue,
+		}},
+	})
 	if !rev.Status.IsReady() {
 		t.Fatalf("Wanted ready revision: %v", rev)
 	}
@@ -612,7 +618,7 @@ func TestIsRevisionStale(t *testing.T) {
 					serving.ConfigurationGenerationLabelKey: "1",
 				},
 				Annotations: map[string]string{
-					"serving.knative.dev/lastPinned":             fmt.Sprintf("%d", staleTime.Unix()),
+					"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
 				},
 			},
 		},
@@ -627,7 +633,7 @@ func TestIsRevisionStale(t *testing.T) {
 					serving.ConfigurationGenerationLabelKey: "1",
 				},
 				Annotations: map[string]string{
-					"serving.knative.dev/lastPinned":             fmt.Sprintf("%d", curTime.Unix()),
+					"serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()),
 				},
 			},
 		},
@@ -642,7 +648,7 @@ func TestIsRevisionStale(t *testing.T) {
 					serving.ConfigurationGenerationLabelKey: "2",
 				},
 				Annotations: map[string]string{
-					"serving.knative.dev/lastPinned":             fmt.Sprintf("%d", staleTime.Unix()),
+					"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
 				},
 			},
 		},
@@ -657,7 +663,7 @@ func TestIsRevisionStale(t *testing.T) {
 					serving.ConfigurationGenerationLabelKey: "1",
 				},
 				Annotations: map[string]string{
-					"serving.knative.dev/lastPinned":             fmt.Sprintf("%d", staleTime.Unix()),
+					"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
 				},
 			},
 		},

diff --git a/pkg/reconciler/v1alpha1/revision/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go
@@ -262,6 +262,13 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 func (c *Reconciler) reconcileBuild(ctx context.Context, rev *v1alpha1.Revision) error {
 	buildRef := rev.BuildRef()
 	if buildRef == nil {
+		rev.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   duckv1alpha1.ConditionSucceeded,
+				Status: corev1.ConditionTrue,
+				Reason: "NoBuild",
+			}},
+		})
 		return nil
 	}
 
@@ -271,7 +278,6 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, rev *v1alpha1.Revision)
 		logger.Errorf("Error tracking build '%+v' for Revision %q: %+v", buildRef, rev.Name, err)
 		return err
 	}
-	rev.Status.InitializeBuildCondition()
 
 	gvr, _ := meta.UnsafeGuessKindToResource(buildRef.GroupVersionKind())
 	_, lister, err := c.buildInformerFactory.Get(gvr)