centralize after calls

Signed-off-by: Scott Andrews <[email protected]>
reconcilerio · Apr 18, 2024 · daa0cac · daa0cac
1 parent f4e5889
commit daa0cac
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 23 deletions.
diff --git a/reconcilers/aggregate.go b/reconcilers/aggregate.go
@@ -257,14 +257,23 @@ func (r *AggregateReconciler[T]) Reconcile(ctx context.Context, req Request) (Re
 	ctx = StashOriginalResourceType(ctx, r.Type)
 	ctx = StashResourceType(ctx, r.Type)
 
-	beforeCtx, result, err := r.BeforeReconcile(ctx, req)
+	beforeCtx, beforeResult, err := r.BeforeReconcile(ctx, req)
 	if err != nil {
-		return result, err
+		return beforeResult, err
 	}
 	if beforeCtx != nil {
 		ctx = beforeCtx
 	}
 
+	reconcileResult, err := r.reconcile(ctx, req)
+
+	return r.AfterReconcile(ctx, req, AggregateResults(beforeResult, reconcileResult), err)
+}
+
+func (r *AggregateReconciler[T]) reconcile(ctx context.Context, req Request) (Result, error) {
+	log := logr.FromContextOrDiscard(ctx)
+	c := RetrieveConfigOrDie(ctx)
+
 	resource := r.Type.DeepCopyObject().(T)
 	if err := c.Get(ctx, req.NamespacedName, resource); err != nil {
 		if apierrs.IsNotFound(err) {
@@ -275,19 +284,18 @@ func (r *AggregateReconciler[T]) Reconcile(ctx context.Context, req Request) (Re
 			if !errors.Is(err, ErrQuiet) {
 				log.Error(err, "unable to fetch resource")
 			}
-			return r.AfterReconcile(ctx, req, result, err)
+			return Result{}, err
 		}
 	}
 
 	if resource.GetDeletionTimestamp() != nil {
 		// resource is being deleted, nothing to do
-		return r.AfterReconcile(ctx, req, result, nil)
+		return Result{}, nil
 	}
 
-	reconcileResult, err := r.Reconciler.Reconcile(ctx, resource)
-	result = AggregateResults(result, reconcileResult)
+	result, err := r.Reconciler.Reconcile(ctx, resource)
 	if err != nil && !errors.Is(err, ErrHaltSubReconcilers) {
-		return r.AfterReconcile(ctx, req, result, err)
+		return result, err
 	}
 
 	// hack, ignore track requests from the child reconciler, we have it covered
@@ -299,13 +307,10 @@ func (r *AggregateReconciler[T]) Reconcile(ctx context.Context, req Request) (Re
 	})
 	desired, err := r.desiredResource(ctx, resource)
 	if err != nil {
-		return r.AfterReconcile(ctx, req, result, err)
+		return result, err
 	}
 	_, err = r.stamp.Manage(ctx, resource, resource, desired)
-	if err != nil {
-		return r.AfterReconcile(ctx, req, result, err)
-	}
-	return r.AfterReconcile(ctx, req, result, nil)
+	return result, err
 }
 
 func (r *AggregateReconciler[T]) desiredResource(ctx context.Context, resource T) (T, error) {

diff --git a/reconcilers/resource.go b/reconcilers/resource.go
@@ -234,28 +234,38 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req Request) (Res
 	ctx = StashOriginalConfig(ctx, c)
 	ctx = StashOriginalResourceType(ctx, r.Type)
 	ctx = StashResourceType(ctx, r.Type)
-	originalResource := r.Type.DeepCopyObject().(T)
 
-	beforeCtx, result, err := r.BeforeReconcile(ctx, req)
+	beforeCtx, beforeResult, err := r.BeforeReconcile(ctx, req)
 	if err != nil {
-		return result, err
+		return beforeResult, err
 	}
 	if beforeCtx != nil {
 		ctx = beforeCtx
 	}
 
+	reconcileResult, err := r.reconcileOuter(ctx, req)
+
+	return r.AfterReconcile(ctx, req, AggregateResults(beforeResult, reconcileResult), err)
+}
+
+func (r *ResourceReconciler[T]) reconcileOuter(ctx context.Context, req Request) (Result, error) {
+	log := logr.FromContextOrDiscard(ctx)
+	c := RetrieveOriginalConfigOrDie(ctx)
+
+	originalResource := r.Type.DeepCopyObject().(T)
+
 	if err := c.Get(ctx, req.NamespacedName, originalResource); err != nil {
 		if apierrs.IsNotFound(err) {
 			// we'll ignore not-found errors, since they can't be fixed by an immediate
 			// requeue (we'll need to wait for a new notification), and we can get them
 			// on deleted requests.
-			return r.AfterReconcile(ctx, req, result, nil)
+			return Result{}, nil
 		}
 		if !errors.Is(err, ErrQuiet) {
 			log.Error(err, "unable to fetch resource")
 		}
 
-		return r.AfterReconcile(ctx, req, result, err)
+		return Result{}, err
 	}
 	resource := originalResource.DeepCopyObject().(T)
 
@@ -265,11 +275,10 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req Request) (Res
 	}
 
 	r.initializeConditions(ctx, resource)
-	reconcileResult, err := r.reconcile(ctx, resource)
-	result = AggregateResults(result, reconcileResult)
+	result, err := r.reconcileInner(ctx, resource)
 
 	if r.SkipStatusUpdate {
-		return r.AfterReconcile(ctx, req, result, err)
+		return result, err
 	}
 
 	// attempt to restore last transition time for unchanged conditions
@@ -288,7 +297,7 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req Request) (Res
 						"Failed to patch status: %v", patchErr)
 				}
 
-				return r.AfterReconcile(ctx, req, result, patchErr)
+				return result, patchErr
 			}
 			c.Recorder.Eventf(resource, corev1.EventTypeNormal, "StatusPatched",
 				"Patched status")
@@ -301,7 +310,7 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req Request) (Res
 					c.Recorder.Eventf(resource, corev1.EventTypeWarning, "StatusUpdateFailed",
 						"Failed to update status: %v", updateErr)
 				}
-				return r.AfterReconcile(ctx, req, result, updateErr)
+				return result, updateErr
 			}
 			c.Recorder.Eventf(resource, corev1.EventTypeNormal, "StatusUpdated",
 				"Updated status")
@@ -312,7 +321,7 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req Request) (Res
 	return result, err
 }
 
-func (r *ResourceReconciler[T]) reconcile(ctx context.Context, resource T) (Result, error) {
+func (r *ResourceReconciler[T]) reconcileInner(ctx context.Context, resource T) (Result, error) {
 	if resource.GetDeletionTimestamp() != nil && len(resource.GetFinalizers()) == 0 {
 		// resource is being deleted and has no pending finalizers, nothing to do
 		return Result{}, nil