fix bug in status updater that could accidentally hide reconciliation errors (#54)

Author: Diaphteiros

pkg/controller/status_updater.go

@@ -177,22 +177,23 @@ func defaultPhaseUpdateFunc[Obj client.Object, PhType ~string, ConType comparabl
 // The object is expected to be a pointer to a struct with the status field.
 // If the 'Object' field in the ReconcileResult is nil, the status update becomes a no-op.
 func (s *statusUpdater[Obj, PhType, ConType]) UpdateStatus(ctx context.Context, c client.Client, rr ReconcileResult[Obj, ConType]) (ctrl.Result, error) {
+	errs := errors.NewReasonableErrorList(rr.ReconcileError)
 	if IsNil(rr.Object) {
-		return rr.Result, nil
+		return rr.Result, errs.Aggregate()
 	}
 	if s.fieldNames[STATUS_FIELD] == "" {
-		return rr.Result, nil
+		return rr.Result, errs.Aggregate()
 	}
 	if IsNil(rr.OldObject) || IsSameObject(rr.OldObject, rr.Object) {
 		// create old object based on given one
 		rr.OldObject = rr.Object.DeepCopyObject().(Obj)
 	}
 	status := GetField(rr.Object, s.fieldNames[STATUS_FIELD], true)
 	if IsNil(status) {
-		return rr.Result, fmt.Errorf("unable to get pointer to status field '%s' of object %T", s.fieldNames[STATUS_FIELD], rr.Object)
+		errs.Append(errors.WithReason(fmt.Errorf("unable to get pointer to status field '%s' of object %T", s.fieldNames[STATUS_FIELD], rr.Object), "InternalError"))
+		return rr.Result, errs.Aggregate()
 	}
 
-	errs := errors.NewReasonableErrorList(rr.ReconcileError)
 	now := time.Now()
 	if s.fieldNames[STATUS_FIELD_LAST_RECONCILE_TIME] != "" {
 		SetField(status, s.fieldNames[STATUS_FIELD_LAST_RECONCILE_TIME], metav1.NewTime(now))

pkg/controller/status_updater_test.go

@@ -64,6 +64,16 @@ var _ = Describe("Status Updater", func() {
 		))
 	})
 
+	It("should not hide a reconciliation error if the object is nil", func() {
+		env := testutils.NewEnvironmentBuilder().WithFakeClient(coScheme).WithInitObjectPath("testdata", "test-02").WithDynamicObjectsWithStatus(&CustomObject{}).Build()
+		rr := controller.ReconcileResult[*CustomObject, ConditionStatus]{
+			ReconcileError: errors.WithReason(fmt.Errorf("test error"), "TestError"),
+		}
+		su := preconfiguredStatusUpdaterBuilder().Build()
+		_, err := su.UpdateStatus(env.Ctx, env.Client(), rr)
+		Expect(err).To(HaveOccurred())
+	})
+
 	It("should update an existing status", func() {
 		env := testutils.NewEnvironmentBuilder().WithFakeClient(coScheme).WithInitObjectPath("testdata", "test-02").WithDynamicObjectsWithStatus(&CustomObject{}).Build()
 		obj := &CustomObject{}