Apimeta Set/RemoveStatusCondition: Indicate change

The SetStatusCondition and RemoveStatusCondition currently do not
indicate if they changed anything. In most cases that information is
necessary to determine if an Update of the object is needed. This change
adds a boolean return to them that indicate if they changed anything.

As the two functions had no return at all prior to this, this shouldn't
break anything.

Kubernetes-commit: 5d56f7cf86f923f0357a2db59e7687f29ec2965c

pkg/api/meta/conditions.go

@@ -22,22 +22,23 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// SetStatusCondition sets the corresponding condition in conditions to newCondition.
+// SetStatusCondition sets the corresponding condition in conditions to newCondition and returns true
+// if the conditions are changed by this call.
 // conditions must be non-nil.
 //  1. if the condition of the specified type already exists (all fields of the existing condition are updated to
 //     newCondition, LastTransitionTime is set to now if the new status differs from the old status)
 //  2. if a condition of the specified type does not exist (LastTransitionTime is set to now() if unset, and newCondition is appended)
-func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Condition) {
+func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Condition) (changed bool) {
 	if conditions == nil {
-		return
+		return false
 	}
 	existingCondition := FindStatusCondition(*conditions, newCondition.Type)
 	if existingCondition == nil {
 		if newCondition.LastTransitionTime.IsZero() {
 			newCondition.LastTransitionTime = metav1.NewTime(time.Now())
 		}
 		*conditions = append(*conditions, newCondition)
-		return
+		return true
 	}
 
 	if existingCondition.Status != newCondition.Status {
@@ -47,18 +48,31 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond
 		} else {
 			existingCondition.LastTransitionTime = metav1.NewTime(time.Now())
 		}
+		changed = true
 	}
 
-	existingCondition.Reason = newCondition.Reason
-	existingCondition.Message = newCondition.Message
-	existingCondition.ObservedGeneration = newCondition.ObservedGeneration
+	if existingCondition.Reason != newCondition.Reason {
+		existingCondition.Reason = newCondition.Reason
+		changed = true
+	}
+	if existingCondition.Message != newCondition.Message {
+		existingCondition.Message = newCondition.Message
+		changed = true
+	}
+	if existingCondition.ObservedGeneration != newCondition.ObservedGeneration {
+		existingCondition.ObservedGeneration = newCondition.ObservedGeneration
+		changed = true
+	}
+
+	return changed
 }
 
-// RemoveStatusCondition removes the corresponding conditionType from conditions.
+// RemoveStatusCondition removes the corresponding conditionType from conditions if present. Returns
+// true if it was present and got removed.
 // conditions must be non-nil.
-func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string) {
+func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string) (removed bool) {
 	if conditions == nil || len(*conditions) == 0 {
-		return
+		return false
 	}
 	newConditions := make([]metav1.Condition, 0, len(*conditions)-1)
 	for _, condition := range *conditions {
@@ -67,7 +81,10 @@ func RemoveStatusCondition(conditions *[]metav1.Condition, conditionType string)
 		}
 	}
 
+	removed = len(*conditions) != len(newConditions)
 	*conditions = newConditions
+
+	return removed
 }
 
 // FindStatusCondition finds the conditionType in conditions.

pkg/api/meta/conditions_test.go

@@ -29,18 +29,20 @@ func TestSetStatusCondition(t *testing.T) {
 	oneHourAfter := time.Now().Add(1 * time.Hour)
 
 	tests := []struct {
-		name       string
-		conditions []metav1.Condition
-		toAdd      metav1.Condition
-		expected   []metav1.Condition
+		name          string
+		conditions    []metav1.Condition
+		toAdd         metav1.Condition
+		expectChanged bool
+		expected      []metav1.Condition
 	}{
 		{
 			name: "should-add",
 			conditions: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
@@ -54,7 +56,8 @@ func TestSetStatusCondition(t *testing.T) {
 				{Type: "second", Status: metav1.ConditionFalse},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message"},
@@ -68,18 +71,36 @@ func TestSetStatusCondition(t *testing.T) {
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}},
 				{Type: "third"},
 			},
-			toAdd: metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourAfter}, Reason: "reason", Message: "message", ObservedGeneration: 3},
+			toAdd:         metav1.Condition{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourAfter}, Reason: "reason", Message: "message", ObservedGeneration: 3},
+			expectChanged: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "second", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}, Reason: "reason", Message: "message", ObservedGeneration: 3},
 				{Type: "third"},
 			},
 		},
+		{
+			name: "nothing changes",
+			conditions: []metav1.Condition{{
+				Type:               "type",
+				Status:             metav1.ConditionTrue,
+				LastTransitionTime: metav1.Time{Time: oneHourBefore},
+			}},
+			toAdd: metav1.Condition{Type: "type", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{Time: oneHourBefore}},
+			expected: []metav1.Condition{{
+				Type:               "type",
+				Status:             metav1.ConditionTrue,
+				LastTransitionTime: metav1.Time{Time: oneHourBefore},
+			}},
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			SetStatusCondition(&test.conditions, test.toAdd)
+			changed := SetStatusCondition(&test.conditions, test.toAdd)
+			if test.expectChanged != changed {
+				t.Errorf("expectChanged=%t != changed=%t", test.expectChanged, changed)
+			}
 			if !reflect.DeepEqual(test.conditions, test.expected) {
 				t.Error(test.conditions)
 			}
@@ -92,6 +113,7 @@ func TestRemoveStatusCondition(t *testing.T) {
 		name          string
 		conditions    []metav1.Condition
 		conditionType string
+		expectRemoval bool
 		expected      []metav1.Condition
 	}{
 		{
@@ -102,6 +124,7 @@ func TestRemoveStatusCondition(t *testing.T) {
 				{Type: "third"},
 			},
 			conditionType: "second",
+			expectRemoval: true,
 			expected: []metav1.Condition{
 				{Type: "first"},
 				{Type: "third"},
@@ -131,7 +154,10 @@ func TestRemoveStatusCondition(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			RemoveStatusCondition(&test.conditions, test.conditionType)
+			removed := RemoveStatusCondition(&test.conditions, test.conditionType)
+			if test.expectRemoval != removed {
+				t.Errorf("expectRemoval=%t != removal=%t", test.expectRemoval, removed)
+			}
 			if !reflect.DeepEqual(test.conditions, test.expected) {
 				t.Error(test.conditions)
 			}