Subscription controller tweaks

pkg/apis/eventing/v1alpha1/subscription_types.go

@@ -17,8 +17,6 @@
 package v1alpha1
 
 import (
-	"encoding/json"
-
 	"github.com/knative/pkg/apis"
 	"github.com/knative/pkg/apis/duck"
 	duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
@@ -55,6 +53,10 @@ var _ duckv1alpha1.ConditionsAccessor = (*SubscriptionStatus)(nil)
 // Check that Subscription implements the Conditions duck type.
 var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Conditions{})
 
+// Check that the Subscription implements the Generation duck type.
+var emptyGen duckv1alpha1.Generation
+var _ = duck.VerifyType(&Subscription{}, &emptyGen)
+
 // And it's Subscribable
 var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Subscribable{})
 
@@ -157,6 +159,8 @@ type Callable struct {
 	TargetURI *string `json:"targetURI,omitempty"`
 }
 
+// ResultStrategy specifies the handling of the Callable's returned result. If
+// no Callable is specified, the identity function is assumed.
 type ResultStrategy struct {
 	// This object must fulfill the Sinkable contract.
 	//
@@ -170,7 +174,9 @@ type ResultStrategy struct {
 	Target *corev1.ObjectReference `json:"target,omitempty"`
 }
 
-var subCondSet = duckv1alpha1.NewLivingConditionSet()
+// subCondSet is a condition set with Ready as the happy condition and
+// ReferencesResolved and FromReady as the dependent conditions.
+var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionFromReady)
 
 // SubscriptionStatus (computed) for a subscription
 type SubscriptionStatus struct {
@@ -188,7 +194,7 @@ const (
 	// SubscriptionConditionReady has status True when all subconditions below have been set to True.
 	SubscriptionConditionReady = duckv1alpha1.ConditionReady
 
-	// SubscriptionReferencesResolved has status True when all the specified references have been successfully
+	// SubscriptionConditionReferencesResolved has status True when all the specified references have been successfully
 	// resolved.
 	SubscriptionConditionReferencesResolved duckv1alpha1.ConditionType = "Resolved"
 
@@ -197,11 +203,6 @@ const (
 	SubscriptionConditionFromReady duckv1alpha1.ConditionType = "FromReady"
 )
 
-// GetSpecJSON returns spec as json
-func (s *Subscription) GetSpecJSON() ([]byte, error) {
-	return json.Marshal(s.Spec)
-}
-
 // GetCondition returns the condition currently associated with the given type, or nil.
 func (ss *SubscriptionStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alpha1.Condition {
 	return subCondSet.Manage(ss).GetCondition(t)
@@ -213,6 +214,26 @@ func (ss *SubscriptionStatus) GetConditions() duckv1alpha1.Conditions {
 	return ss.Conditions
 }
 
+// IsReady returns true if the resource is ready overall.
+func (ss *SubscriptionStatus) IsReady() bool {
+	return subCondSet.Manage(ss).IsHappy()
+}
+
+// InitializeConditions sets relevant unset conditions to Unknown state.
+func (ss *SubscriptionStatus) InitializeConditions() {
+	subCondSet.Manage(ss).InitializeConditions()
+}
+
+// MarkReferencesResolved sets the ReferencesResolved condition to True state.
+func (ss *SubscriptionStatus) MarkReferencesResolved() {
+	subCondSet.Manage(ss).MarkTrue(SubscriptionConditionReferencesResolved)
+}
+
+// MarkFromReady sets the FromReady condition to True state.
+func (ss *SubscriptionStatus) MarkFromReady() {
+	subCondSet.Manage(ss).MarkTrue(SubscriptionConditionFromReady)
+}
+
 // SetConditions sets the Conditions array. This enables generic handling of
 // conditions by implementing the duckv1alpha1.Conditions interface.
 func (ss *SubscriptionStatus) SetConditions(conditions duckv1alpha1.Conditions) {

pkg/apis/eventing/v1alpha1/subscription_types_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 )
@@ -42,12 +43,12 @@ var subscriptionConditionFromReady = duckv1alpha1.Condition{
 func TestSubscriptionGetCondition(t *testing.T) {
 	tests := []struct {
 		name      string
-		cs        *SubscriptionStatus
+		ss        *SubscriptionStatus
 		condQuery duckv1alpha1.ConditionType
 		want      *duckv1alpha1.Condition
 	}{{
 		name: "single condition",
-		cs: &SubscriptionStatus{
+		ss: &SubscriptionStatus{
 			Conditions: []duckv1alpha1.Condition{
 				subscriptionConditionReady,
 			},
@@ -56,7 +57,7 @@ func TestSubscriptionGetCondition(t *testing.T) {
 		want:      &subscriptionConditionReady,
 	}, {
 		name: "multiple conditions",
-		cs: &SubscriptionStatus{
+		ss: &SubscriptionStatus{
 			Conditions: []duckv1alpha1.Condition{
 				subscriptionConditionReady,
 				subscriptionConditionReferencesResolved,
@@ -66,7 +67,7 @@ func TestSubscriptionGetCondition(t *testing.T) {
 		want:      &subscriptionConditionReferencesResolved,
 	}, {
 		name: "multiple conditions, condition true",
-		cs: &SubscriptionStatus{
+		ss: &SubscriptionStatus{
 			Conditions: []duckv1alpha1.Condition{
 				subscriptionConditionReady,
 				subscriptionConditionFromReady,
@@ -76,7 +77,7 @@ func TestSubscriptionGetCondition(t *testing.T) {
 		want:      &subscriptionConditionFromReady,
 	}, {
 		name: "unknown condition",
-		cs: &SubscriptionStatus{
+		ss: &SubscriptionStatus{
 			Conditions: []duckv1alpha1.Condition{
 				subscriptionConditionReady,
 				subscriptionConditionReferencesResolved,
@@ -88,14 +89,131 @@ func TestSubscriptionGetCondition(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := test.cs.GetCondition(test.condQuery)
+			got := test.ss.GetCondition(test.condQuery)
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Errorf("unexpected condition (-want, +got) = %v", diff)
 			}
 		})
 	}
 }
 
+func TestSubscriptionInitializeConditions(t *testing.T) {
+	tests := []struct {
+		name string
+		ss   *SubscriptionStatus
+		want *SubscriptionStatus
+	}{{
+		name: "empty",
+		ss:   &SubscriptionStatus{},
+		want: &SubscriptionStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   SubscriptionConditionFromReady,
+				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   SubscriptionConditionReady,
+				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   SubscriptionConditionReferencesResolved,
+				Status: corev1.ConditionUnknown,
+			}},
+		},
+	}, {
+		name: "one false",
+		ss: &SubscriptionStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   SubscriptionConditionFromReady,
+				Status: corev1.ConditionFalse,
+			}},
+		},
+		want: &SubscriptionStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   SubscriptionConditionFromReady,
+				Status: corev1.ConditionFalse,
+			}, {
+				Type:   SubscriptionConditionReady,
+				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   SubscriptionConditionReferencesResolved,
+				Status: corev1.ConditionUnknown,
+			}},
+		},
+	}, {
+		name: "one true",
+		ss: &SubscriptionStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   SubscriptionConditionReferencesResolved,
+				Status: corev1.ConditionTrue,
+			}},
+		},
+		want: &SubscriptionStatus{
+			Conditions: []duckv1alpha1.Condition{{
+				Type:   SubscriptionConditionFromReady,
+				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   SubscriptionConditionReady,
+				Status: corev1.ConditionUnknown,
+			}, {
+				Type:   SubscriptionConditionReferencesResolved,
+				Status: corev1.ConditionTrue,
+			}},
+		},
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			test.ss.InitializeConditions()
+			ignore := cmpopts.IgnoreFields(duckv1alpha1.Condition{}, "LastTransitionTime")
+			if diff := cmp.Diff(test.want, test.ss, ignore); diff != "" {
+				t.Errorf("unexpected conditions (-want, +got) = %v", diff)
+			}
+		})
+	}
+}
+
+func TestSubscriptionIsReady(t *testing.T) {
+	tests := []struct {
+		name          string
+		markResolved  bool
+		markFromReady bool
+		wantReady     bool
+	}{{
+		name:          "all happy",
+		markResolved:  true,
+		markFromReady: true,
+		wantReady:     true,
+	}, {
+		name:          "one sad",
+		markResolved:  false,
+		markFromReady: true,
+		wantReady:     false,
+	}, {
+		name:          "other sad",
+		markResolved:  true,
+		markFromReady: false,
+		wantReady:     false,
+	}, {
+		name:          "both sad",
+		markResolved:  false,
+		markFromReady: false,
+		wantReady:     false,
+	}}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ss := &SubscriptionStatus{}
+			if test.markResolved {
+				ss.MarkReferencesResolved()
+			}
+			if test.markFromReady {
+				ss.MarkFromReady()
+			}
+			got := ss.IsReady()
+			if test.wantReady != got {
+				t.Errorf("unexpected readiness: want %v, got %v", test.wantReady, got)
+			}
+		})
+	}
+}
+
 func TestSubscriptionSetConditions(t *testing.T) {
 	c := &Subscription{
 		Status: SubscriptionStatus{},
@@ -107,32 +225,3 @@ func TestSubscriptionSetConditions(t *testing.T) {
 		t.Errorf("unexpected conditions (-want, +got) = %v", diff)
 	}
 }
-
-func TestSubscriptionGetSpecJSON(t *testing.T) {
-	targetURI := "http://example.com"
-	c := &Subscription{
-		Spec: SubscriptionSpec{
-			From: corev1.ObjectReference{
-				Name: "foo",
-			},
-			Call: &Callable{
-				TargetURI: &targetURI,
-			},
-			Result: &ResultStrategy{
-				Target: &corev1.ObjectReference{
-					Name: "result",
-				},
-			},
-		},
-	}
-
-	want := `{"from":{"name":"foo"},"call":{"targetURI":"http://example.com"},"result":{"target":{"name":"result"}}}`
-	got, err := c.GetSpecJSON()
-	if err != nil {
-		t.Fatalf("unexpected spec JSON error: %v", err)
-	}
-
-	if diff := cmp.Diff(want, string(got)); diff != "" {
-		t.Errorf("unexpected spec JSON (-want, +got) = %v", diff)
-	}
-}

pkg/controller/eventing/subscription/reconcile.go

@@ -78,9 +78,7 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 }
 
 func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error {
-	// TODO: Should this just also set up a defer call for subscription.SetConditions.
-	// No time right now as I'm turning into a pumpking but seems reasonable.
-	conditions := []duckv1alpha1.Condition{}
+	subscription.Status.InitializeConditions()
 
 	// See if the subscription has been deleted
 	accessor, err := meta.Accessor(subscription)
@@ -130,10 +128,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error {
 	}
 
 	// Everything that was supposed to be resolved was, so flip the status bit on that.
-	conditions = append(conditions, duckv1alpha1.Condition{
-		Type:   v1alpha1.SubscriptionConditionReferencesResolved,
-		Status: corev1.ConditionTrue,
-	})
+	subscription.Status.MarkReferencesResolved()
 
 	// Ok, now that we have the From and at least one of the Call/Result, let's reconcile
 	// the From with this information.
@@ -143,11 +138,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error {
 		return err
 	}
 	// Everything went well, set the fact that subscriptions have been modified
-	conditions = append(conditions, duckv1alpha1.Condition{
-		Type:   duckv1alpha1.ConditionReady,
-		Status: corev1.ConditionTrue,
-	})
-	subscription.Status.SetConditions(conditions)
+	subscription.Status.MarkFromReady()
 	return nil
 }
 

pkg/controller/eventing/subscription/reconcile_test.go

@@ -234,9 +234,10 @@ var testCases = []controllertesting.TestCase{
 			// TODO: Again this works on gke cluster, but I need to set
 			// something else up here. later...
 			// getNewSubscriptionWithReferencesResolvedStatus(),
-			getNewSubscription(),
+			getNewSubscriptionWithUnknownConditions(),
 		},
-		Scheme: scheme.Scheme,
+		IgnoreTimes: true,
+		Scheme:      scheme.Scheme,
 		Objects: []runtime.Object{
 			// Source channel
 			&unstructured.Unstructured{
@@ -412,15 +413,6 @@ func getNewChannel(name string) *eventingv1alpha1.Channel {
 	return channel
 }
 
-func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription {
-	s := getNewSubscription()
-	s.Status.SetConditions([]duckv1alpha1.Condition{{
-		Type:   eventingv1alpha1.SubscriptionConditionReferencesResolved,
-		Status: corev1.ConditionTrue,
-	}})
-	return s
-}
-
 func getNewSubscription() *eventingv1alpha1.Subscription {
 	subscription := &eventingv1alpha1.Subscription{
 		TypeMeta:   subscriptionType(),
@@ -454,6 +446,18 @@ func getNewSubscription() *eventingv1alpha1.Subscription {
 	return subscription
 }
 
+func getNewSubscriptionWithUnknownConditions() *eventingv1alpha1.Subscription {
+	s := getNewSubscription()
+	s.Status.InitializeConditions()
+	return s
+}
+
+func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription {
+	s := getNewSubscriptionWithUnknownConditions()
+	s.Status.MarkReferencesResolved()
+	return s
+}
+
 func channelType() metav1.TypeMeta {
 	return metav1.TypeMeta{
 		APIVersion: eventingv1alpha1.SchemeGroupVersion.String(),