From 316b59ee888e3adabb9c64fdea44457eade0ad8b Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 9 Sep 2023 15:41:58 -0400 Subject: [PATCH] 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 | 37 +++++++++++++++++++-------- pkg/api/meta/conditions_test.go | 44 ++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/pkg/api/meta/conditions.go b/pkg/api/meta/conditions.go index 60c8209de..cbdf2eeb8 100644 --- a/pkg/api/meta/conditions.go +++ b/pkg/api/meta/conditions.go @@ -22,14 +22,15 @@ 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 { @@ -37,7 +38,7 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond 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. diff --git a/pkg/api/meta/conditions_test.go b/pkg/api/meta/conditions_test.go index 72e5f680c..248c88580 100644 --- a/pkg/api/meta/conditions_test.go +++ b/pkg/api/meta/conditions_test.go @@ -29,10 +29,11 @@ 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", @@ -40,7 +41,8 @@ func TestSetStatusCondition(t *testing.T) { {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) }