From f798777a3b1eae14247046e552e98297c97f7778 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:13:50 -0500 Subject: [PATCH 1/9] Refactor: reorder parameters to MergeConditions --- .../jwtcachefiller/jwtcachefiller.go | 4 ++-- .../webhookcachefiller/webhookcachefiller.go | 4 ++-- .../controller/conditionsutil/conditions_util.go | 15 ++++++++++----- .../conditionsutil/conditions_util_test.go | 4 ++-- .../active_directory_upstream_watcher.go | 8 +++++++- .../supervisorconfig/federation_domain_watcher.go | 9 +++++++-- .../github_upstream_watcher.go | 4 ++-- .../ldapupstreamwatcher/ldap_upstream_watcher.go | 8 +++++++- .../oidcclientwatcher/oidc_client_watcher.go | 9 +++++++-- .../oidcupstreamwatcher/oidc_upstream_watcher.go | 8 +++++++- 10 files changed, 53 insertions(+), 20 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 64ec4ffc1..a84ad931d 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -735,10 +735,10 @@ func (c *jwtCacheFillerController) updateStatus( _ = conditionsutil.MergeConditions( conditions, - original.Generation, &updated.Status.Conditions, - logger, + original.Generation, metav1.NewTime(c.clock.Now()), + logger, ) if equality.Semantic.DeepEqual(original, updated) { diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index f80dad113..eacddfc4c 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -527,10 +527,10 @@ func (c *webhookCacheFillerController) updateStatus( _ = conditionsutil.MergeConditions( conditions, - original.Generation, &updated.Status.Conditions, - logger, + original.Generation, metav1.NewTime(c.clock.Now()), + logger, ) if equality.Semantic.DeepEqual(original, updated) { diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index fc90c3986..436f4ac81 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -24,22 +24,27 @@ const ( ) // MergeConditions merges conditions into conditionsToUpdate. -// Note that LastTransitionTime refers to the time when the status changed, -// but ObservedGeneration should be the current generation for all conditions, since Pinniped should always check every condition. +// Note that lastTransitionTime refers to the time when the status changed, +// but observedGeneration should be the current generation for all conditions, +// since Pinniped should always check every condition. // It returns true if any resulting condition has non-true status. func MergeConditions( conditions []*metav1.Condition, - observedGeneration int64, conditionsToUpdate *[]metav1.Condition, - log plog.MinLogger, + observedGeneration int64, lastTransitionTime metav1.Time, + log plog.MinLogger, ) bool { for i := range conditions { cond := conditions[i].DeepCopy() cond.LastTransitionTime = lastTransitionTime cond.ObservedGeneration = observedGeneration if mergeCondition(conditionsToUpdate, cond) { - log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message) + log.Info("updated condition", + "type", cond.Type, + "status", cond.Status, + "reason", cond.Reason, + "message", cond.Message) } } sort.SliceStable(*conditionsToUpdate, func(i, j int) bool { diff --git a/internal/controller/conditionsutil/conditions_util_test.go b/internal/controller/conditionsutil/conditions_util_test.go index ceb4aee53..b3c473287 100644 --- a/internal/controller/conditionsutil/conditions_util_test.go +++ b/internal/controller/conditionsutil/conditions_util_test.go @@ -172,10 +172,10 @@ func TestMergeIDPConditions(t *testing.T) { result := MergeConditions( tt.newConditions, - tt.observedGeneration, tt.conditionsToUpdate, - logger, + tt.observedGeneration, testTime, + logger, ) logString := log.String() diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 0026126be..4511b8407 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -382,7 +382,13 @@ func (c *activeDirectoryWatcherController) updateStatus(ctx context.Context, ups log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() - hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now()) + hadErrorCondition := conditionsutil.MergeConditions( + conditions, + &updated.Status.Conditions, + upstream.Generation, + metav1.Now(), + log, + ) updated.Status.Phase = idpv1alpha1.ActiveDirectoryPhaseReady if hadErrorCondition { diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 715b2b36f..8d60306c3 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -829,8 +829,13 @@ func (c *federationDomainWatcherController) updateStatus( }) } - _ = conditionsutil.MergeConditions(conditions, - federationDomain.Generation, &updated.Status.Conditions, plog.New().WithName(controllerName), metav1.NewTime(c.clock.Now())) + _ = conditionsutil.MergeConditions( + conditions, + &updated.Status.Conditions, + federationDomain.Generation, + metav1.NewTime(c.clock.Now()), + plog.New().WithName(controllerName), + ) if equality.Semantic.DeepEqual(federationDomain, updated) { return nil diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 1f8236cac..e90c1fc66 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -569,10 +569,10 @@ func (c *gitHubWatcherController) updateStatus( hadErrorCondition := conditionsutil.MergeConditions( conditions, - upstream.Generation, &updated.Status.Conditions, - log, + upstream.Generation, metav1.NewTime(c.clock.Now()), + log, ) updated.Status.Phase = idpv1alpha1.GitHubPhaseReady diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index f97fa77a9..5c8c23b9f 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -268,7 +268,13 @@ func (c *ldapWatcherController) updateStatus(ctx context.Context, upstream *idpv log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() - hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now()) + hadErrorCondition := conditionsutil.MergeConditions( + conditions, + &updated.Status.Conditions, + upstream.Generation, + metav1.Now(), + log, + ) updated.Status.Phase = idpv1alpha1.LDAPPhaseReady if hadErrorCondition { diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index 4086f3532..a9573ba42 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go @@ -133,8 +133,13 @@ func (c *oidcClientWatcherController) updateStatus( ) error { updated := upstream.DeepCopy() - hadErrorCondition := conditionsutil.MergeConditions(conditions, - upstream.Generation, &updated.Status.Conditions, plog.New(), metav1.Now()) + hadErrorCondition := conditionsutil.MergeConditions( + conditions, + &updated.Status.Conditions, + upstream.Generation, + metav1.Now(), + plog.New(), + ) updated.Status.Phase = supervisorconfigv1alpha1.OIDCClientPhaseReady if hadErrorCondition { diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 3c01188e8..e6ab2f448 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -460,7 +460,13 @@ func (c *oidcWatcherController) updateStatus( log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() - hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now()) + hadErrorCondition := conditionsutil.MergeConditions( + conditions, + &updated.Status.Conditions, + upstream.Generation, + metav1.Now(), + log, + ) updated.Status.Phase = idpv1alpha1.PhaseReady if hadErrorCondition { From 6bf30bc6b5d7bedc285806700ed6a4e342055bbb Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:20:35 -0500 Subject: [PATCH 2/9] Backfill test for existing exported function HadErrorCondition --- .../conditionsutil/conditions_util_test.go | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/internal/controller/conditionsutil/conditions_util_test.go b/internal/controller/conditionsutil/conditions_util_test.go index b3c473287..f5402286d 100644 --- a/internal/controller/conditionsutil/conditions_util_test.go +++ b/internal/controller/conditionsutil/conditions_util_test.go @@ -188,3 +188,77 @@ func TestMergeIDPConditions(t *testing.T) { }) } } + +func TestHadErrorCondition(t *testing.T) { + tests := []struct { + name string + conditions []*metav1.Condition + wantResult bool + }{ + { + name: "Returns false when all conditions have status true", + conditions: []*metav1.Condition{ + { + Status: metav1.ConditionTrue, + }, + { + Status: metav1.ConditionTrue, + }, + }, + wantResult: false, + }, + { + name: "Returns false when input is nil", + conditions: nil, + wantResult: false, + }, + { + name: "Returns false when input is empty", + conditions: []*metav1.Condition{}, + wantResult: false, + }, + { + name: "Returns true when any condition has status unknown", + conditions: []*metav1.Condition{ + { + Status: metav1.ConditionTrue, + }, + { + Status: metav1.ConditionUnknown, + }, + }, + wantResult: true, + }, + { + name: "Returns true when any condition has status false", + conditions: []*metav1.Condition{ + { + Status: metav1.ConditionTrue, + }, + { + Status: metav1.ConditionFalse, + }, + }, + wantResult: true, + }, + { + name: "Returns true when any condition has invalid status", + conditions: []*metav1.Condition{ + { + Status: metav1.ConditionTrue, + }, + { + Status: "not a valid status", + }, + }, + wantResult: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := HadErrorCondition(test.conditions) + require.Equal(t, test.wantResult, actual) + }) + } +} From 17f66331ea71d0162bb3c3aece6fe75f5220cc53 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:30:28 -0500 Subject: [PATCH 3/9] Refactor parameter names for clarity --- .../conditionsutil/conditions_util.go | 38 +++++++++--------- .../conditionsutil/conditions_util_test.go | 40 +++++++++---------- 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 436f4ac81..63cde9f75 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -29,17 +29,17 @@ const ( // since Pinniped should always check every condition. // It returns true if any resulting condition has non-true status. func MergeConditions( - conditions []*metav1.Condition, - conditionsToUpdate *[]metav1.Condition, + newConditions []*metav1.Condition, + existingConditionsToUpdate *[]metav1.Condition, observedGeneration int64, lastTransitionTime metav1.Time, log plog.MinLogger, ) bool { - for i := range conditions { - cond := conditions[i].DeepCopy() + for i := range newConditions { + cond := newConditions[i].DeepCopy() cond.LastTransitionTime = lastTransitionTime cond.ObservedGeneration = observedGeneration - if mergeCondition(conditionsToUpdate, cond) { + if mergeCondition(existingConditionsToUpdate, cond) { log.Info("updated condition", "type", cond.Type, "status", cond.Status, @@ -47,39 +47,39 @@ func MergeConditions( "message", cond.Message) } } - sort.SliceStable(*conditionsToUpdate, func(i, j int) bool { - return (*conditionsToUpdate)[i].Type < (*conditionsToUpdate)[j].Type + sort.SliceStable(*existingConditionsToUpdate, func(i, j int) bool { + return (*existingConditionsToUpdate)[i].Type < (*existingConditionsToUpdate)[j].Type }) - return HadErrorCondition(conditions) + return HadErrorCondition(newConditions) } // mergeCondition merges a new metav1.Condition into a slice of existing conditions. It returns true -// if the condition has meaningfully changed. -func mergeCondition(existing *[]metav1.Condition, new *metav1.Condition) bool { +// if something other than the LastTransitionTime has been updated. +func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition *metav1.Condition) bool { // Find any existing condition with a matching type. var old *metav1.Condition - for i := range *existing { - if (*existing)[i].Type == new.Type { - old = &(*existing)[i] + for i := range *existingConditionsToUpdate { + if (*existingConditionsToUpdate)[i].Type == newCondition.Type { + old = &(*existingConditionsToUpdate)[i] continue } } // If there is no existing condition of this type, append this one and we're done. if old == nil { - *existing = append(*existing, *new) + *existingConditionsToUpdate = append(*existingConditionsToUpdate, *newCondition) return true } // Set the LastTransitionTime depending on whether the status has changed. - new = new.DeepCopy() - if old.Status == new.Status { - new.LastTransitionTime = old.LastTransitionTime + newCondition = newCondition.DeepCopy() + if old.Status == newCondition.Status { + newCondition.LastTransitionTime = old.LastTransitionTime } // If anything has actually changed, update the entry and return true. - if !equality.Semantic.DeepEqual(old, new) { - *old = *new + if !equality.Semantic.DeepEqual(old, newCondition) { + *old = *newCondition return true } diff --git a/internal/controller/conditionsutil/conditions_util_test.go b/internal/controller/conditionsutil/conditions_util_test.go index f5402286d..7c34a81b4 100644 --- a/internal/controller/conditionsutil/conditions_util_test.go +++ b/internal/controller/conditionsutil/conditions_util_test.go @@ -21,13 +21,13 @@ func TestMergeIDPConditions(t *testing.T) { testTime := metav1.Now() tests := []struct { - name string - newConditions []*metav1.Condition - conditionsToUpdate *[]metav1.Condition - observedGeneration int64 - wantResult bool - wantLogSnippets []string - wantConditions []metav1.Condition + name string + newConditions []*metav1.Condition + existingConditionsToUpdate *[]metav1.Condition + observedGeneration int64 + wantResult bool + wantLogSnippets []string + wantConditions []metav1.Condition }{ { name: "Adding a new condition with status=True returns false", @@ -39,8 +39,8 @@ func TestMergeIDPConditions(t *testing.T) { Message: "new message", }, }, - observedGeneration: int64(999), - conditionsToUpdate: &[]metav1.Condition{}, + observedGeneration: int64(999), + existingConditionsToUpdate: &[]metav1.Condition{}, wantLogSnippets: []string{ `"message":"updated condition","type":"NewType","status":"True"`, }, @@ -78,7 +78,7 @@ func TestMergeIDPConditions(t *testing.T) { Message: "new message", }, }, - conditionsToUpdate: &[]metav1.Condition{ + existingConditionsToUpdate: &[]metav1.Condition{ { Type: "UnchangedType", Status: metav1.ConditionTrue, @@ -140,7 +140,7 @@ func TestMergeIDPConditions(t *testing.T) { Message: "unchanged message", }, }, - conditionsToUpdate: &[]metav1.Condition{ + existingConditionsToUpdate: &[]metav1.Condition{ { Type: "UnchangedType", Status: metav1.ConditionFalse, @@ -165,26 +165,26 @@ func TestMergeIDPConditions(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) result := MergeConditions( - tt.newConditions, - tt.conditionsToUpdate, - tt.observedGeneration, + test.newConditions, + test.existingConditionsToUpdate, + test.observedGeneration, testTime, logger, ) logString := log.String() - require.Equal(t, len(tt.wantLogSnippets), strings.Count(logString, "\n")) - for _, wantLog := range tt.wantLogSnippets { + require.Equal(t, len(test.wantLogSnippets), strings.Count(logString, "\n")) + for _, wantLog := range test.wantLogSnippets { require.Contains(t, logString, wantLog) } - require.Equal(t, tt.wantResult, result) - require.Equal(t, tt.wantConditions, *tt.conditionsToUpdate) + require.Equal(t, test.wantResult, result) + require.Equal(t, test.wantConditions, *test.existingConditionsToUpdate) }) } } From 2d8ab9ff5d997d127d78df895b972c5784d750fd Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:35:24 -0500 Subject: [PATCH 4/9] Refactor variable name for clarity --- .../controller/conditionsutil/conditions_util.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 63cde9f75..da6b3a2ef 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -57,29 +57,29 @@ func MergeConditions( // if something other than the LastTransitionTime has been updated. func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition *metav1.Condition) bool { // Find any existing condition with a matching type. - var old *metav1.Condition + var existingCondition *metav1.Condition for i := range *existingConditionsToUpdate { if (*existingConditionsToUpdate)[i].Type == newCondition.Type { - old = &(*existingConditionsToUpdate)[i] + existingCondition = &(*existingConditionsToUpdate)[i] continue } } // If there is no existing condition of this type, append this one and we're done. - if old == nil { + if existingCondition == nil { *existingConditionsToUpdate = append(*existingConditionsToUpdate, *newCondition) return true } // Set the LastTransitionTime depending on whether the status has changed. newCondition = newCondition.DeepCopy() - if old.Status == newCondition.Status { - newCondition.LastTransitionTime = old.LastTransitionTime + if existingCondition.Status == newCondition.Status { + newCondition.LastTransitionTime = existingCondition.LastTransitionTime } // If anything has actually changed, update the entry and return true. - if !equality.Semantic.DeepEqual(old, newCondition) { - *old = *newCondition + if !equality.Semantic.DeepEqual(existingCondition, newCondition) { + *existingCondition = *newCondition return true } From 1e8e9ecc98ebe60a827b0f37bff4731231170303 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:46:42 -0500 Subject: [PATCH 5/9] Refactor to use slices helpers instead of harder-to-read loops --- .../conditionsutil/conditions_util.go | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index da6b3a2ef..d21cfa5f7 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -4,6 +4,7 @@ package conditionsutil import ( + "slices" "sort" "k8s.io/apimachinery/pkg/api/equality" @@ -57,18 +58,17 @@ func MergeConditions( // if something other than the LastTransitionTime has been updated. func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition *metav1.Condition) bool { // Find any existing condition with a matching type. - var existingCondition *metav1.Condition - for i := range *existingConditionsToUpdate { - if (*existingConditionsToUpdate)[i].Type == newCondition.Type { - existingCondition = &(*existingConditionsToUpdate)[i] - continue - } - } + index := slices.IndexFunc(*existingConditionsToUpdate, func(condition metav1.Condition) bool { + return newCondition.Type == condition.Type + }) - // If there is no existing condition of this type, append this one and we're done. - if existingCondition == nil { + var existingCondition *metav1.Condition + if index < 0 { + // If there is no existing condition of this type, append this one and we're done. *existingConditionsToUpdate = append(*existingConditionsToUpdate, *newCondition) return true + } else { + existingCondition = &(*existingConditionsToUpdate)[index] } // Set the LastTransitionTime depending on whether the status has changed. @@ -88,10 +88,7 @@ func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition } func HadErrorCondition(conditions []*metav1.Condition) bool { - for _, c := range conditions { - if c.Status != metav1.ConditionTrue { - return true - } - } - return false + return slices.ContainsFunc(conditions, func(condition *metav1.Condition) bool { + return condition.Status != metav1.ConditionTrue + }) } From 8b97414f3d3eea4cce7940e446b18f1003d4cd78 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:55:37 -0500 Subject: [PATCH 6/9] Refactor to simplify logic --- .../controller/conditionsutil/conditions_util.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index d21cfa5f7..306a2f5d7 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -7,7 +7,7 @@ import ( "slices" "sort" - "k8s.io/apimachinery/pkg/api/equality" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/internal/plog" @@ -77,14 +77,9 @@ func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition newCondition.LastTransitionTime = existingCondition.LastTransitionTime } - // If anything has actually changed, update the entry and return true. - if !equality.Semantic.DeepEqual(existingCondition, newCondition) { - *existingCondition = *newCondition - return true - } - - // Otherwise the entry is already up-to-date. - return false + changed := !apiequality.Semantic.DeepEqual(existingCondition, newCondition) + *existingCondition = *newCondition + return changed } func HadErrorCondition(conditions []*metav1.Condition) bool { From 4a9136040ce52cbc6ebbeb6cc2a01351afbf2090 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 10:56:07 -0500 Subject: [PATCH 7/9] Refactor to make it obvious that newCondition is a copy --- .../conditionsutil/conditions_util.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 306a2f5d7..128b0ddac 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -37,15 +37,15 @@ func MergeConditions( log plog.MinLogger, ) bool { for i := range newConditions { - cond := newConditions[i].DeepCopy() - cond.LastTransitionTime = lastTransitionTime - cond.ObservedGeneration = observedGeneration - if mergeCondition(existingConditionsToUpdate, cond) { + newCondition := newConditions[i].DeepCopy() + newCondition.LastTransitionTime = lastTransitionTime + newCondition.ObservedGeneration = observedGeneration + if mergeCondition(existingConditionsToUpdate, newCondition) { log.Info("updated condition", - "type", cond.Type, - "status", cond.Status, - "reason", cond.Reason, - "message", cond.Message) + "type", newCondition.Type, + "status", newCondition.Status, + "reason", newCondition.Reason, + "message", newCondition.Message) } } sort.SliceStable(*existingConditionsToUpdate, func(i, j int) bool { @@ -72,8 +72,7 @@ func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition } // Set the LastTransitionTime depending on whether the status has changed. - newCondition = newCondition.DeepCopy() - if existingCondition.Status == newCondition.Status { + if newCondition.Status == existingCondition.Status { newCondition.LastTransitionTime = existingCondition.LastTransitionTime } From 4bd5db14b45caef5a18959827da86187b0ca83ef Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 7 Aug 2024 12:23:11 -0500 Subject: [PATCH 8/9] Refactor branching logic when using an early return --- internal/controller/conditionsutil/conditions_util.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 128b0ddac..7729c604b 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -67,11 +67,12 @@ func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition // If there is no existing condition of this type, append this one and we're done. *existingConditionsToUpdate = append(*existingConditionsToUpdate, *newCondition) return true - } else { - existingCondition = &(*existingConditionsToUpdate)[index] } - // Set the LastTransitionTime depending on whether the status has changed. + // Get a pointer to the existing condition + existingCondition = &(*existingConditionsToUpdate)[index] + + // If the status has not changed, preserve the original lastTransitionTime if newCondition.Status == existingCondition.Status { newCondition.LastTransitionTime = existingCondition.LastTransitionTime } From bab8b54ed89e67946a3f8a38ff3cf084d816a9d5 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Thu, 8 Aug 2024 08:16:04 -0500 Subject: [PATCH 9/9] Update godoc --- internal/controller/conditionsutil/conditions_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 7729c604b..ee2f2434f 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -24,7 +24,7 @@ const ( MessageUnableToValidate = "unable to validate; see other conditions for details" ) -// MergeConditions merges conditions into conditionsToUpdate. +// MergeConditions merges newConditions into existingConditionsToUpdate. // Note that lastTransitionTime refers to the time when the status changed, // but observedGeneration should be the current generation for all conditions, // since Pinniped should always check every condition.