From 21b01619ab2092814352455ada7b709a818f7509 Mon Sep 17 00:00:00 2001
From: Yuxin Bai <LittleBaiBai@users.noreply.github.com>
Date: Wed, 24 Jan 2024 02:33:48 -0500
Subject: [PATCH] Update original cast resource even on reconciliation error
 (#474)

* Update original cast resource even on reconciliation error

Closes #473
---
 reconcilers/cast.go      | 12 ++++++---
 reconcilers/cast_test.go | 57 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/reconcilers/cast.go b/reconcilers/cast.go
index 24686e0..66f7fce 100644
--- a/reconcilers/cast.go
+++ b/reconcilers/cast.go
@@ -9,6 +9,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"k8s.io/apimachinery/pkg/util/errors"
 	"reflect"
 	"sync"
 
@@ -111,22 +112,25 @@ func (r *CastResource[T, CT]) Reconcile(ctx context.Context, resource T) (Result
 	}
 	castOriginal := castResource.DeepCopyObject().(client.Object)
 	result, err := r.Reconciler.Reconcile(ctx, castResource)
+	var errs []error
 	if err != nil {
-		return result, err
+		errs = append(errs, err)
 	}
 	if !equality.Semantic.DeepEqual(castResource, castOriginal) {
 		// patch the reconciled resource with the updated duck values
 		patch, err := NewPatch(castOriginal, castResource)
 		if err != nil {
-			return Result{}, err
+			errs = append(errs, err)
+			return result, errors.NewAggregate(errs)
 		}
 		err = patch.Apply(resource)
 		if err != nil {
-			return Result{}, err
+			errs = append(errs, err)
+			return result, errors.NewAggregate(errs)
 		}
 
 	}
-	return result, nil
+	return result, errors.NewAggregate(errs)
 }
 
 func (r *CastResource[T, CT]) cast(ctx context.Context, resource T) (context.Context, CT, error) {
diff --git a/reconcilers/cast_test.go b/reconcilers/cast_test.go
index d0a7624..ee47e53 100644
--- a/reconcilers/cast_test.go
+++ b/reconcilers/cast_test.go
@@ -114,13 +114,19 @@ func TestCastResource(t *testing.T) {
 			},
 			ExpectedResult: reconcilers.Result{Requeue: true},
 		},
-		"return subreconciler err, preserves result": {
+		"return subreconciler err, preserves result and status update": {
 			Resource: resource.DieReleasePtr(),
 			Metadata: map[string]interface{}{
 				"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] {
 					return &reconcilers.CastResource[*resources.TestResource, *appsv1.Deployment]{
 						Reconciler: &reconcilers.SyncReconciler[*appsv1.Deployment]{
 							SyncWithResult: func(ctx context.Context, resource *appsv1.Deployment) (reconcilers.Result, error) {
+								resource.Status.Conditions[0] = appsv1.DeploymentCondition{
+									Type:    apis.ConditionReady,
+									Status:  corev1.ConditionFalse,
+									Reason:  "Failed",
+									Message: "expected error",
+								}
 								return reconcilers.Result{Requeue: true}, fmt.Errorf("subreconciler error")
 							},
 						},
@@ -128,7 +134,14 @@ func TestCastResource(t *testing.T) {
 				},
 			},
 			ExpectedResult: reconcilers.Result{Requeue: true},
-			ShouldErr:      true,
+			ExpectResource: resource.
+				StatusDie(func(d *dies.TestResourceStatusDie) {
+					d.ConditionsDie(
+						diemetav1.ConditionBlank.Type(apis.ConditionReady).Status(metav1.ConditionFalse).Reason("Failed").Message("expected error"),
+					)
+				}).
+				DieReleasePtr(),
+			ShouldErr: true,
 		},
 		"marshal error": {
 			Resource: resource.
@@ -170,6 +183,46 @@ func TestCastResource(t *testing.T) {
 			},
 			ShouldErr: true,
 		},
+		"cast mutation patch error": {
+			Resource: resource.DieReleasePtr(),
+			Metadata: map[string]interface{}{
+				"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] {
+					return &reconcilers.CastResource[*resources.TestResource, *resources.TestResource]{
+						Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
+							Sync: func(ctx context.Context, r *resources.TestResource) error {
+								r.Spec.ErrOnMarshal = true
+								return fmt.Errorf("subreconciler error")
+							},
+						},
+					}
+				},
+			},
+			ShouldErr: true,
+		},
+		"cast mutation patch apply error": {
+			Resource: resource.DieReleasePtr(),
+			Metadata: map[string]interface{}{
+				"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] {
+					return &reconcilers.CastResource[*resources.TestResource, *resources.TestResource]{
+						Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
+							Sync: func(ctx context.Context, r *resources.TestResource) error {
+								r.Spec.ErrOnUnmarshal = true
+								return fmt.Errorf("subreconciler error")
+							},
+						},
+					}
+				},
+			},
+			ExpectResource: resource.
+				SpecDie(func(d *dies.TestResourceSpecDie) {
+					d.ErrOnUnmarshal(true)
+				}).
+				StatusDie(func(d *dies.TestResourceStatusDie) {
+					d.ConditionsDie() // The unmarshal error would result in losing the initializing Ready condition during applying the patch
+				}).
+				DieReleasePtr(),
+			ShouldErr: true,
+		},
 	}
 
 	rts.Run(t, scheme, func(t *testing.T, rtc *rtesting.SubReconcilerTestCase[*resources.TestResource], c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] {