Consolidate result conversion and computation

Consolidate BuildRuntimeResult() into summarizeAndPatch() to simplify
where the results are computed, summarized and patched.

Move the event recording and logging of context specific errors into
RecordContextualError() and call it in summarizeAndPatch().

Introduce Waiting error for wait and requeue scenarios. Update
ComputeReconcileResult() and RecordContextualError() to consider Waiting
error.

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

controllers/bucket_controller.go

@@ -139,20 +139,19 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		return ctrl.Result{}, nil
 	}
 
-	// Initialize the patch helper
+	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
+	// recResult stores the abstracted reconcile result.
 	var recResult sreconcile.Result
 
 	// Always attempt to patch the object and status after each reconciliation
-	// NOTE: This deferred block only modifies the named return error. The
-	// result from the reconciliation remains the same. Any requeue attributes
-	// set in the result will continue to be effective.
+	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -163,13 +162,13 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
 		recResult = sreconcile.ResultRequeue
-		return ctrl.Result{Requeue: true}, nil
+		return
 	}
 
 	// Examine if the object is under deletion
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		res, err := r.reconcileDelete(ctx, obj)
-		return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err)
+		recResult, retErr = r.reconcileDelete(ctx, obj)
+		return
 	}
 
 	// Reconcile actual object
@@ -178,21 +177,30 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		r.reconcileSource,
 		r.reconcileArtifact,
 	}
-	recResult, err = r.reconcile(ctx, obj, reconcilers)
-	return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err)
+	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	return
 }
 
 // summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions and patches the object with the calculated summary.
-func (r *BucketReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.Bucket, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error {
+// status conditions, computes runtime results and patches the object in the K8s
+// API server.
+func (r *BucketReconciler) summarizeAndPatch(
+	ctx context.Context,
+	obj *sourcev1.Bucket,
+	patchHelper *patch.Helper,
+	res sreconcile.Result,
+	recErr error) (ctrl.Result, error) {
+	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
+
 	// Record the value of the reconciliation request if any.
 	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
 		obj.Status.SetLastHandledReconcileRequest(v)
 	}
 
 	// Compute the reconcile results, obtain patch options and reconcile error.
 	var patchOpts []patch.Option
-	patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, bucketOwnedConditions)
+	var result ctrl.Result
+	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, bucketOwnedConditions)
 
 	// Summarize the Ready condition based on abnormalities that may have been observed.
 	conditions.SetSummary(obj,
@@ -214,7 +222,7 @@ func (r *BucketReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.
 		recErr = kerrors.NewAggregate([]error{recErr, err})
 	}
 
-	return recErr
+	return result, recErr
 }
 
 // reconcile steps iterates through the actual reconciliation tasks for objec,

controllers/gitrepository_controller.go

@@ -145,20 +145,19 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, nil
 	}
 
-	// Initialize the patch helper
+	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
+	// recResult stores the abstracted reconcile result.
 	var recResult sreconcile.Result
 
 	// Always attempt to patch the object and status after each reconciliation
-	// NOTE: This deferred block only modifies the named return error. The
-	// result from the reconciliation remains the same. Any requeue attributes
-	// set in the result will continue to be effective.
+	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -170,13 +169,13 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
 		recResult = sreconcile.ResultRequeue
-		return ctrl.Result{Requeue: true}, nil
+		return
 	}
 
 	// Examine if the object is under deletion
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		res, err := r.reconcileDelete(ctx, obj)
-		return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err)
+		recResult, retErr = r.reconcileDelete(ctx, obj)
+		return
 	}
 
 	// Reconcile actual object
@@ -186,21 +185,30 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		r.reconcileInclude,
 		r.reconcileArtifact,
 	}
-	recResult, err = r.reconcile(ctx, obj, reconcilers)
-	return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err)
+	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	return
 }
 
 // summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions and patches the object with the calculated summary.
-func (r *GitRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.GitRepository, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error {
+// status conditions, computes runtime results and patches the object in the K8s
+// API server.
+func (r *GitRepositoryReconciler) summarizeAndPatch(
+	ctx context.Context,
+	obj *sourcev1.GitRepository,
+	patchHelper *patch.Helper,
+	res sreconcile.Result,
+	recErr error) (ctrl.Result, error) {
+	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
+
 	// Record the value of the reconciliation request if any.
 	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
 		obj.Status.SetLastHandledReconcileRequest(v)
 	}
 
 	// Compute the reconcile results, obtain patch options and reconcile error.
 	var patchOpts []patch.Option
-	patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, gitRepoOwnedConditions)
+	var result ctrl.Result
+	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, gitRepoOwnedConditions)
 
 	// Summarize the Ready condition based on abnormalities that may have been observed.
 	conditions.SetSummary(obj,
@@ -222,7 +230,7 @@ func (r *GitRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *so
 		recErr = kerrors.NewAggregate([]error{recErr, err})
 	}
 
-	return recErr
+	return result, recErr
 }
 
 // reconcile steps iterates through the actual reconciliation tasks for objec,

controllers/helmchart_controller.go

@@ -169,21 +169,19 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, nil
 	}
 
-	// Initialize the patch helper
+	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// Result of the sub-reconciliation
+	// recResult stores the abstracted reconcile result.
 	var recResult sreconcile.Result
 
 	// Always attempt to patch the object after each reconciliation.
-	// NOTE: This deferred block only modifies the named return error. The
-	// result from the reconciliation remains the same. Any requeue attributes
-	// set in the result will continue to be effective.
+	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -195,13 +193,13 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
 		recResult = sreconcile.ResultRequeue
-		return ctrl.Result{Requeue: true}, nil
+		return
 	}
 
 	// Examine if the object is under deletion
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		res, err := r.reconcileDelete(ctx, obj)
-		return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err)
+		recResult, retErr = r.reconcileDelete(ctx, obj)
+		return
 	}
 
 	// Reconcile actual object
@@ -210,23 +208,30 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		r.reconcileSource,
 		r.reconcileArtifact,
 	}
-	recResult, err = r.reconcile(ctx, obj, reconcilers)
-	return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err)
+	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	return
 }
 
 // summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions and patches the object with the calculated summary. The
-// reconciler error type is also used to determine the conditions and the
-// returned error.
-func (r *HelmChartReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.HelmChart, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error {
+// status conditions, computes runtime results and patches the object in the K8s
+// API server.
+func (r *HelmChartReconciler) summarizeAndPatch(
+	ctx context.Context,
+	obj *sourcev1.HelmChart,
+	patchHelper *patch.Helper,
+	res sreconcile.Result,
+	recErr error) (ctrl.Result, error) {
+	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
+
 	// Record the value of the reconciliation request, if any
 	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
 		obj.Status.SetLastHandledReconcileRequest(v)
 	}
 
 	// Compute the reconcile results, obtain patch options and reconcile error.
 	var patchOpts []patch.Option
-	patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, helmChartOwnedConditions)
+	var result ctrl.Result
+	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, helmChartOwnedConditions)
 
 	// Summarize Ready condition
 	conditions.SetSummary(obj,
@@ -247,7 +252,7 @@ func (r *HelmChartReconciler) summarizeAndPatch(ctx context.Context, obj *source
 		}
 		recErr = kerrors.NewAggregate([]error{recErr, err})
 	}
-	return recErr
+	return result, recErr
 }
 
 // reconcile steps through the actual reconciliation tasks for the object, it returns early on the first step that

controllers/helmchart_controller_test.go

@@ -1372,7 +1372,8 @@ func TestHelmChartReconciler_summarizeAndPatch(t *testing.T) {
 
 			builder := fake.NewClientBuilder().WithScheme(testEnv.GetScheme())
 			r := &HelmChartReconciler{
-				Client: builder.Build(),
+				Client:        builder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
 			}
 			obj := &sourcev1.HelmChart{
 				ObjectMeta: metav1.ObjectMeta{
@@ -1393,7 +1394,7 @@ func TestHelmChartReconciler_summarizeAndPatch(t *testing.T) {
 			patchHelper, err := patch.NewHelper(obj, r.Client)
 			g.Expect(err).ToNot(HaveOccurred())
 
-			gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr)
+			_, gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr)
 			g.Expect(gotErr != nil).To(Equal(tt.wantErr))
 
 			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))

controllers/helmrepository_controller.go

@@ -132,21 +132,19 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{}, nil
 	}
 
-	// Initialize the patch helper
+	// Initialize the patch helper with the current version of the object.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// Result of the sub-reconciliation.
+	// recResult stores the abstracted reconcile result.
 	var recResult sreconcile.Result
 
 	// Always attempt to patch the object after each reconciliation.
-	// NOTE: This deferred block only modifies the named return error. The
-	// result from the reconciliation remains the same. Any requeue attributes
-	// set in the result will continue to be effective.
+	// NOTE: The final runtime result and error are set in this block.
 	defer func() {
-		retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
+		result, retErr = r.summarizeAndPatch(ctx, obj, patchHelper, recResult, retErr)
 
 		// Always record readiness and duration metrics
 		r.Metrics.RecordReadiness(ctx, obj)
@@ -158,13 +156,13 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	if !controllerutil.ContainsFinalizer(obj, sourcev1.SourceFinalizer) {
 		controllerutil.AddFinalizer(obj, sourcev1.SourceFinalizer)
 		recResult = sreconcile.ResultRequeue
-		return ctrl.Result{Requeue: true}, nil
+		return
 	}
 
 	// Examine if the object is under deletion
 	if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		res, err := r.reconcileDelete(ctx, obj)
-		return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, res, err)
+		recResult, retErr = r.reconcileDelete(ctx, obj)
+		return
 	}
 
 	// Reconcile actual object
@@ -173,23 +171,30 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		r.reconcileSource,
 		r.reconcileArtifact,
 	}
-	recResult, err = r.reconcile(ctx, obj, reconcilers)
-	return sreconcile.BuildRuntimeResult(ctx, r.EventRecorder, obj, recResult, err)
+	recResult, retErr = r.reconcile(ctx, obj, reconcilers)
+	return
 }
 
 // summarizeAndPatch analyzes the object conditions to create a summary of the
-// status conditions and patches the object with the calculated summary. The
-// reconciler error type is also used to determine the conditions and the
-// returned error.
-func (r *HelmRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.HelmRepository, patchHelper *patch.Helper, res sreconcile.Result, recErr error) error {
+// status conditions, computes runtime results and patches the object in the K8s
+// API server.
+func (r *HelmRepositoryReconciler) summarizeAndPatch(
+	ctx context.Context,
+	obj *sourcev1.HelmRepository,
+	patchHelper *patch.Helper,
+	res sreconcile.Result,
+	recErr error) (ctrl.Result, error) {
+	sreconcile.RecordContextualError(ctx, r.EventRecorder, obj, recErr)
+
 	// Record the value of the reconciliation request, if any.
 	if v, ok := meta.ReconcileAnnotationValue(obj.GetAnnotations()); ok {
 		obj.Status.SetLastHandledReconcileRequest(v)
 	}
 
 	// Compute the reconcile results, obtain patch options and reconcile error.
 	var patchOpts []patch.Option
-	patchOpts, recErr = sreconcile.ComputeReconcileResult(obj, res, recErr, helmRepoOwnedConditions)
+	var result ctrl.Result
+	patchOpts, result, recErr = sreconcile.ComputeReconcileResult(obj, obj.GetRequeueAfter(), res, recErr, helmRepoOwnedConditions)
 
 	// Summarize Ready condition.
 	conditions.SetSummary(obj,
@@ -211,7 +216,7 @@ func (r *HelmRepositoryReconciler) summarizeAndPatch(ctx context.Context, obj *s
 		recErr = kerrors.NewAggregate([]error{recErr, err})
 	}
 
-	return recErr
+	return result, recErr
 }
 
 // reconcile iterates through the sub-reconcilers and processes the source

controllers/helmrepository_controller_test.go

@@ -752,7 +752,8 @@ func TestHelmRepositoryReconciler_summarizeAndPatch(t *testing.T) {
 
 			builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
 			r := &HelmRepositoryReconciler{
-				Client: builder.Build(),
+				Client:        builder.Build(),
+				EventRecorder: record.NewFakeRecorder(32),
 			}
 			obj := &sourcev1.HelmRepository{
 				ObjectMeta: metav1.ObjectMeta{
@@ -773,7 +774,7 @@ func TestHelmRepositoryReconciler_summarizeAndPatch(t *testing.T) {
 			patchHelper, err := patch.NewHelper(obj, r.Client)
 			g.Expect(err).ToNot(HaveOccurred())
 
-			gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr)
+			_, gotErr := r.summarizeAndPatch(ctx, obj, patchHelper, tt.result, tt.reconcileErr)
 			g.Expect(gotErr != nil).To(Equal(tt.wantErr))
 
 			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))