refactor(object-controller): isolate standalone manager PR

Move Secret retry behavior and revision-status wording to focused branches. Keep this PR scoped to manager setup and its integration coverage.

Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

Author: fao89

cmd/object-controller/main_test.go

@@ -135,7 +135,6 @@ func TestStandaloneController(t *testing.T) {
 				progressing := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
 				if assert.NotNil(collect, progressing) {
 					assert.Equal(collect, ocv1.ReasonSucceeded, progressing.Reason)
-					assert.Equal(collect, "Revision 1 has rolled out.", progressing.Message)
 				}
 			}, time.Minute, 100*time.Millisecond)
 			cm := &corev1.ConfigMap{}

internal/object-controller/controllers/clusterobjectset_controller.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -131,20 +132,14 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
 	remaining, hasDeadline := durationUntilDeadline(c.Clock, cos)
 	isDeadlineExceeded := hasDeadline && remaining <= 0
 
-	secretReader := newReferencedSecretReader(c.Client)
 	// Blocked takes precedence over ProgressDeadlineExceeded: it is more actionable for the user.
-	if err := c.verifyReferencedSecretsImmutable(ctx, cos, secretReader); err != nil {
-		var mutableSecrets *mutableSecretsError
-		if !errors.As(err, &mutableSecrets) {
-			setRetryingConditions(l, cos, err.Error(), isDeadlineExceeded)
-			return ctrl.Result{}, err
-		}
+	if err := c.verifyReferencedSecretsImmutable(ctx, cos); err != nil {
 		l.Error(err, "referenced Secret verification failed, blocking reconciliation")
 		markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonBlocked, err.Error())
-		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
+		return ctrl.Result{}, nil
 	}
 
-	phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos, secretReader)
+	phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos)
 	if err != nil {
 		setRetryingConditions(l, cos, err.Error(), isDeadlineExceeded)
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
@@ -230,9 +225,6 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
 	}
 
 	revVersion := cos.GetAnnotations()[labels.BundleVersionKey]
-	if revVersion == "" {
-		revVersion = fmt.Sprint(cos.Spec.Revision)
-	}
 	if rres.InTransition() {
 		markAsProgressing(l, cos, ocv1.ReasonRollingOut, fmt.Sprintf("Revision %s is rolling out.", revVersion), isDeadlineExceeded)
 	}
@@ -475,7 +467,7 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions(
 	return result, nil
 }
 
-func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet, secretReader *referencedSecretReader) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) {
+func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) {
 	siblings, err := c.listSiblingRevisions(ctx, cos)
 	if err != nil {
 		return nil, nil, nil, fmt.Errorf("listing sibling revisions: %w", err)
@@ -507,7 +499,7 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
 			case specObj.Object.Object != nil:
 				obj = specObj.Object.DeepCopy()
 			case specObj.Ref.Name != "":
-				resolved, err := secretReader.resolveObjectRef(ctx, specObj.Ref)
+				resolved, err := c.resolveObjectRef(ctx, specObj.Ref)
 				if err != nil {
 					return nil, nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err)
 				}
@@ -549,10 +541,10 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
 
 // resolveObjectRef fetches the referenced Secret, reads the value at the specified key,
 // auto-detects gzip compression, and deserializes into an unstructured.Unstructured.
-func (r *referencedSecretReader) resolveObjectRef(ctx context.Context, ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, error) {
+func (c *ClusterObjectSetReconciler) resolveObjectRef(ctx context.Context, ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, error) {
+	secret := &corev1.Secret{}
 	key := client.ObjectKey{Name: ref.Name, Namespace: ref.Namespace}
-	secret, err := r.get(ctx, key)
-	if err != nil {
+	if err := c.Client.Get(ctx, key, secret); err != nil {
 		return nil, fmt.Errorf("getting Secret %s/%s: %w", ref.Namespace, ref.Name, err)
 	}
 
@@ -785,7 +777,7 @@ func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error {
 // verifyReferencedSecretsImmutable checks that all referenced Secrets
 // have Immutable set to true. It collects all violations and returns
 // a single error listing every misconfigured Secret.
-func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet, secretReader *referencedSecretReader) error {
+func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet) error {
 	type secretRef struct {
 		name      string
 		namespace string
@@ -808,9 +800,9 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
 
 	var mutableSecrets []string
 	for _, ref := range refs {
+		secret := &corev1.Secret{}
 		key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace}
-		secret, err := secretReader.get(ctx, key)
-		if err != nil {
+		if err := c.Client.Get(ctx, key, secret); err != nil {
 			if apierrors.IsNotFound(err) {
 				// Secret not yet available — skip verification.
 				// resolveObjectRef will handle the not-found with a retryable error.
@@ -825,7 +817,8 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
 	}
 
 	if len(mutableSecrets) > 0 {
-		return &mutableSecretsError{names: mutableSecrets}
+		return fmt.Errorf("the following secrets are not immutable (referenced secrets must have immutable set to true): %s",
+			strings.Join(mutableSecrets, ", "))
 	}
 
 	return nil

internal/object-controller/controllers/clusterobjectset_controller_internal_test.go

@@ -436,7 +436,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
 			},
 		}
 
-		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
+		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
 		require.NoError(t, err)
 	})
 
@@ -466,7 +466,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
 			},
 		}
 
-		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
+		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "not immutable")
 	})
@@ -489,7 +489,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
 			},
 		}
 
-		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
+		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
 		require.NoError(t, err)
 	})
 
@@ -523,7 +523,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
 			},
 		}
 
-		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
+		err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
 		require.NoError(t, err)
 		assert.Equal(t, int32(1), secretGetCount.Load(), "secret should be fetched only once despite multiple references")
 	})