From 823b0ffc808cdba06b7e21ff5c40250f4bf4b17a Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Tue, 22 Jul 2025 11:14:27 +0200 Subject: [PATCH 1/4] fix bug in condition updater (wrongfully updated condition) --- pkg/conditions/updater.go | 13 +++++++------ pkg/conditions/updater_test.go | 10 ++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/conditions/updater.go b/pkg/conditions/updater.go index f53d52b..0cfa209 100644 --- a/pkg/conditions/updater.go +++ b/pkg/conditions/updater.go @@ -96,11 +96,6 @@ func (c *conditionUpdater) UpdateCondition(conType string, status metav1.Conditi ObservedGeneration: observedGeneration, LastTransitionTime: c.Now, } - old, ok := c.conditions[conType] - if ok && old.Status == con.Status { - // update LastTransitionTime only if status changed - con.LastTransitionTime = old.LastTransitionTime - } c.updates[conType] = status c.conditions[conType] = con return c @@ -134,7 +129,13 @@ func (c *conditionUpdater) RemoveCondition(conType string) *conditionUpdater { // The conditions are returned sorted by their type. // The second return value indicates whether the condition list has actually changed. func (c *conditionUpdater) Conditions() ([]metav1.Condition, bool) { - res := c.updatedConditions() + res := collections.ProjectSlice(c.updatedConditions(), func(con metav1.Condition) metav1.Condition { + if c.original[con.Type].Status == con.Status { + // if the status has not changed, reset the LastTransitionTime to the original value + con.LastTransitionTime = c.original[con.Type].LastTransitionTime + } + return con + }) slices.SortStableFunc(res, func(a, b metav1.Condition) int { return strings.Compare(a.Type, b.Type) }) diff --git a/pkg/conditions/updater_test.go b/pkg/conditions/updater_test.go index 8deaf69..faa1b31 100644 --- a/pkg/conditions/updater_test.go +++ b/pkg/conditions/updater_test.go @@ -161,6 +161,16 @@ var _ = Describe("Conditions", func() { Expect(newCon.LastTransitionTime.After(oldCon.LastTransitionTime.Time)).To(BeTrue()) }) + It("should not change the last transition time if the same condition is updated multiple times with the last update setting it to the original value again", func() { + cons := testConditionSet() + oldCon := conditions.GetCondition(cons, "true") + updater := conditions.ConditionUpdater(cons, false) + updated, changed := updater.UpdateCondition(oldCon.Type, invert(oldCon.Status), oldCon.ObservedGeneration+1, "newReason", "newMessage"). + UpdateCondition(oldCon.Type, oldCon.Status, oldCon.ObservedGeneration, oldCon.Reason, oldCon.Message).Conditions() + Expect(changed).To(BeFalse()) + Expect(updated).To(ConsistOf(cons)) + }) + It("should sort the conditions by type", func() { cons := []metav1.Condition{ TestConditionFromValues("c", conditions.FromBool(true), 0, "reason", "message", metav1.Now()).ToCondition(), From 194d1c2216fd70c5d3e9313c94a09683dcb81394 Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Wed, 23 Jul 2025 12:27:27 +0200 Subject: [PATCH 2/4] enforce reason --- pkg/conditions/updater.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/conditions/updater.go b/pkg/conditions/updater.go index 0cfa209..fc47495 100644 --- a/pkg/conditions/updater.go +++ b/pkg/conditions/updater.go @@ -88,6 +88,10 @@ func (c *conditionUpdater) WithEventRecorder(recorder record.EventRecorder, verb // All fields of the condition are updated with the values given in the arguments, but the condition's LastTransitionTime is only updated (with the timestamp contained in the receiver struct) if the status changed. // Returns the receiver for easy chaining. func (c *conditionUpdater) UpdateCondition(conType string, status metav1.ConditionStatus, observedGeneration int64, reason, message string) *conditionUpdater { + if reason == "" { + // the metav1.Condition type requires a reason, so let's add a dummy if none is given + reason = conType + string(status) + } con := metav1.Condition{ Type: conType, Status: status, From 11e9e361c8c0bdea2c9a785e7cade0c69fa06722 Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Wed, 23 Jul 2025 12:47:02 +0200 Subject: [PATCH 3/4] add GenerateCreateConditionFunc function to controller package --- pkg/controller/status_updater.go | 24 +++++++++++++++- pkg/controller/status_updater_test.go | 41 +++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/pkg/controller/status_updater.go b/pkg/controller/status_updater.go index 326cb63..f57488d 100644 --- a/pkg/controller/status_updater.go +++ b/pkg/controller/status_updater.go @@ -231,7 +231,11 @@ func (s *statusUpdater[Obj]) UpdateStatus(ctx context.Context, c client.Client, } cu.Now = now for _, con := range rr.Conditions { - cu.UpdateConditionFromTemplate(con) + gen := con.ObservedGeneration + if gen == 0 { + gen = rr.Object.GetGeneration() + } + cu.UpdateCondition(con.Type, con.Status, gen, con.Reason, con.Message) } newCons, _ := cu.Record(rr.Object).Conditions() SetField(status, s.fieldNames[STATUS_FIELD_CONDITIONS], newCons) @@ -366,3 +370,21 @@ type ReconcileResult[Obj client.Object] struct { // The lastTransition timestamp of the condition will be overwritten with the current time while updating. Conditions []metav1.Condition } + +// GenerateCreateConditionFunc returns a function that can be used to add a condition to the given ReconcileResult. +// If the ReconcileResult's Object is not nil, the condition's ObservedGeneration is set to the object's generation. +func GenerateCreateConditionFunc[Obj client.Object](rr *ReconcileResult[Obj]) func(conType string, status metav1.ConditionStatus, reason, message string) { + var gen int64 = 0 + if any(rr.Object) != nil { + gen = rr.Object.GetGeneration() + } + return func(conType string, status metav1.ConditionStatus, reason, message string) { + rr.Conditions = append(rr.Conditions, metav1.Condition{ + Type: conType, + Status: status, + ObservedGeneration: gen, + Reason: reason, + Message: message, + }) + } +} diff --git a/pkg/controller/status_updater_test.go b/pkg/controller/status_updater_test.go index 9fa042c..786af50 100644 --- a/pkg/controller/status_updater_test.go +++ b/pkg/controller/status_updater_test.go @@ -194,6 +194,47 @@ var _ = Describe("Status Updater", func() { } }) + Context("GenerateCreateConditionFunc", func() { + + It("should add the condition to the given ReconcileResult", func() { + rr := controller.ReconcileResult[*CustomObject]{ + Object: &CustomObject{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + Generation: 15, + }, + }, + Conditions: dummyConditions(), + } + createCon := controller.GenerateCreateConditionFunc(&rr) + createCon("TestConditionFoo", metav1.ConditionTrue, "TestReasonFoo", "TestMessageFoo") + Expect(rr.Conditions).To(ConsistOf( + MatchCondition(TestConditionFromCondition(dummyConditions()[0])), + MatchCondition(TestConditionFromCondition(dummyConditions()[1])), + MatchCondition(TestConditionFromValues("TestConditionFoo", metav1.ConditionTrue, 15, "TestReasonFoo", "TestMessageFoo", metav1.Time{})), + )) + }) + + It("should create the condition list, if it is nil", func() { + rr := controller.ReconcileResult[*CustomObject]{ + Object: &CustomObject{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + Generation: 15, + }, + }, + } + createCon := controller.GenerateCreateConditionFunc(&rr) + createCon("TestConditionFoo", metav1.ConditionTrue, "TestReasonFoo", "TestMessageFoo") + Expect(rr.Conditions).To(ConsistOf( + MatchCondition(TestConditionFromValues("TestConditionFoo", metav1.ConditionTrue, 15, "TestReasonFoo", "TestMessageFoo", metav1.Time{})), + )) + }) + + }) + }) func preconfiguredStatusUpdaterBuilder() *controller.StatusUpdaterBuilder[*CustomObject] { From fcaffddd1d4f2d7531ae2de2f63c714cb40508d2 Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Wed, 23 Jul 2025 12:47:10 +0200 Subject: [PATCH 4/4] feat: release v0.13.1 --- VERSION | 2 +- go.mod | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index d92a93b..d609fec 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v0.13.0-dev \ No newline at end of file +v0.13.1 \ No newline at end of file diff --git a/go.mod b/go.mod index 1b186a7..a927fd8 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/onsi/ginkgo v1.16.5 github.com/onsi/ginkgo/v2 v2.23.4 github.com/onsi/gomega v1.37.0 - github.com/openmcp-project/controller-utils/api v0.13.0 + github.com/openmcp-project/controller-utils/api v0.13.1 github.com/spf13/pflag v1.0.6 github.com/stretchr/testify v1.10.0 go.uber.org/zap v1.27.0