Merge pull request #1016 from fluxcd/condn-checker-with-t

Improve HelmRepository type switching from default to oci

controllers/bucket_controller_test.go

@@ -120,7 +120,7 @@ func TestBucketReconciler_Reconcile(t *testing.T) {
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: bucketReadyCondition.NegativePolarity}
 	checker := conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	// kstatus client conformance check.
 	uo, err := patch.ToUnstructured(obj)
@@ -321,7 +321,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -662,7 +662,7 @@ func TestBucketReconciler_reconcileSource_generic(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -1022,7 +1022,7 @@ func TestBucketReconciler_reconcileSource_gcs(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -1201,7 +1201,7 @@ func TestBucketReconciler_reconcileArtifact(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }

controllers/gitrepository_controller_test.go

@@ -186,7 +186,7 @@ func TestGitRepositoryReconciler_Reconcile(t *testing.T) {
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: gitRepositoryReadyCondition.NegativePolarity}
 	checker := conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	// kstatus client conformance check.
 	u, err := patch.ToUnstructured(obj)
@@ -590,7 +590,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -815,7 +815,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 			}
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -1374,7 +1374,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }

controllers/helmchart_controller_test.go

@@ -109,7 +109,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) {
 				// Check if the object status is valid.
 				condns := &conditionscheck.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
 				checker := conditionscheck.NewChecker(testEnv.Client, condns)
-				checker.CheckErr(ctx, obj)
+				checker.WithT(g).CheckErr(ctx, obj)
 
 				// kstatus client conformance check.
 				u, err := patch.ToUnstructured(obj)
@@ -177,7 +177,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) {
 				// Check if the object status is valid.
 				condns := &conditionscheck.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
 				checker := conditionscheck.NewChecker(testEnv.Client, condns)
-				checker.CheckErr(ctx, obj)
+				checker.WithT(g).CheckErr(ctx, obj)
 
 				g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
@@ -212,7 +212,7 @@ func TestHelmChartReconciler_Reconcile(t *testing.T) {
 				// Check if the object status is valid.
 				condns := &conditionscheck.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
 				checker := conditionscheck.NewChecker(testEnv.Client, condns)
-				checker.CheckErr(ctx, obj)
+				checker.WithT(g).CheckErr(ctx, obj)
 
 				g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
@@ -444,7 +444,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -708,7 +708,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, &obj)
+			checker.WithT(g).CheckErr(ctx, &obj)
 		})
 	}
 }

controllers/helmrepository_controller_oci.go

@@ -40,6 +40,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
+	eventv1 "github.com/fluxcd/pkg/apis/event/v1beta1"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/oci"
 	"github.com/fluxcd/pkg/runtime/conditions"
@@ -82,6 +83,11 @@ type HelmRepositoryOCIReconciler struct {
 	RegistryClientGenerator RegistryClientGeneratorFunc
 
 	patchOptions []patch.Option
+
+	// unmanagedConditions are the conditions that are not managed by this
+	// reconciler and need to be removed from the object before taking ownership
+	// of the object being reconciled.
+	unmanagedConditions []string
 }
 
 // RegistryClientGeneratorFunc is a function that returns a registry client
@@ -95,6 +101,7 @@ func (r *HelmRepositoryOCIReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 func (r *HelmRepositoryOCIReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmRepositoryReconcilerOptions) error {
+	r.unmanagedConditions = conditionsDiff(helmRepositoryReadyCondition.Owned, helmRepositoryOCIOwnedConditions)
 	r.patchOptions = getPatchOptions(helmRepositoryOCIOwnedConditions, r.ControllerName)
 
 	recoverPanic := true
@@ -124,6 +131,16 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// If the object contains any of the unmanaged conditions, requeue and wait
+	// for those conditions to be removed first before processing the object.
+	// NOTE: This will happen only when a HelmRepository's spec.type is switched
+	// from "default" to "oci".
+	if conditions.HasAny(obj, r.unmanagedConditions) {
+		r.eventLogf(ctx, obj, eventv1.EventTypeTrace, "IncompleteTransition",
+			"object contains conditions managed by other reconciler")
+		return ctrl.Result{RequeueAfter: time.Second}, nil
+	}
+
 	// Record suspended status metric
 	r.RecordSuspend(ctx, obj, obj.Spec.Suspend)
 
@@ -428,3 +445,18 @@ func makeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registry
 
 	return nil, nil
 }
+
+func conditionsDiff(a, b []string) []string {
+	bMap := make(map[string]struct{}, len(b))
+	for _, j := range b {
+		bMap[j] = struct{}{}
+	}
+
+	r := []string{}
+	for _, i := range a {
+		if _, exists := bMap[i]; !exists {
+			r = append(r, i)
+		}
+	}
+	return r
+}

controllers/helmrepository_controller_oci_test.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"encoding/base64"
 	"fmt"
+	"strconv"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -122,7 +123,7 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
 			// Check if the object status is valid.
 			condns := &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity}
 			checker := conditionscheck.NewChecker(testEnv.Client, condns)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 
 			// kstatus client conformance check.
 			u, err := patch.ToUnstructured(obj)
@@ -316,7 +317,28 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) {
 			// NOTE: Check the object directly as reconcile() doesn't apply the
 			// final patch, the object has unapplied changes.
 			checker.DisableFetch = true
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
+		})
+	}
+}
+
+func TestConditionsDiff(t *testing.T) {
+	tests := []struct {
+		a, b, want []string
+	}{
+		{[]string{"a", "b", "c"}, []string{"b", "d"}, []string{"a", "c"}},
+		{[]string{"a", "b", "c"}, []string{}, []string{"a", "b", "c"}},
+		{[]string{}, []string{"b", "d"}, []string{}},
+		{[]string{}, []string{}, []string{}},
+		{[]string{"a", "b"}, nil, []string{"a", "b"}},
+		{nil, []string{"a", "b"}, []string{}},
+		{nil, nil, []string{}},
+	}
+
+	for i, tt := range tests {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			g := NewWithT(t)
+			g.Expect(conditionsDiff(tt.a, tt.b)).To(Equal(tt.want))
 		})
 	}
 }

controllers/helmrepository_controller_test.go

@@ -94,7 +94,7 @@ func TestHelmRepositoryReconciler_Reconcile(t *testing.T) {
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity}
 	checker := conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	// kstatus client conformance check.
 	u, err := patch.ToUnstructured(obj)
@@ -292,7 +292,7 @@ func TestHelmRepositoryReconciler_reconcileStorage(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -746,7 +746,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }
@@ -1278,7 +1278,7 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity}
 	checker := conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	// kstatus client conformance check.
 	u, err := patch.ToUnstructured(obj)
@@ -1330,7 +1330,7 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 	// Check if the object status is valid.
 	condns = &conditionscheck.Conditions{NegativePolarity: helmRepositoryOCINegativeConditions}
 	checker = conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
@@ -1395,7 +1395,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
 	// Check if the object status is valid.
 	condns := &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity}
 	checker := conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	// kstatus client conformance check.
 	u, err := patch.ToUnstructured(obj)
@@ -1427,7 +1427,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
 	// Check if the object status is valid.
 	condns = &conditionscheck.Conditions{NegativePolarity: helmRepositoryReadyCondition.NegativePolarity}
 	checker = conditionscheck.NewChecker(testEnv.Client, condns)
-	checker.CheckErr(ctx, obj)
+	checker.WithT(g).CheckErr(ctx, obj)
 
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 

controllers/ocirepository_controller_test.go

@@ -217,7 +217,7 @@ func TestOCIRepository_Reconcile(t *testing.T) {
 			// Check if the object status is valid
 			condns := &conditionscheck.Conditions{NegativePolarity: ociRepositoryReadyCondition.NegativePolarity}
 			checker := conditionscheck.NewChecker(testEnv.Client, condns)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 
 			// kstatus client conformance check
 			u, err := patch.ToUnstructured(obj)
@@ -1998,7 +1998,7 @@ func TestOCIRepository_reconcileStorage(t *testing.T) {
 
 			// In-progress status condition validity.
 			checker := conditionscheck.NewInProgressChecker(r.Client)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }

go.mod

@@ -31,7 +31,7 @@ require (
 	github.com/fluxcd/pkg/lockedfile v0.1.0
 	github.com/fluxcd/pkg/masktoken v0.2.0
 	github.com/fluxcd/pkg/oci v0.18.0
-	github.com/fluxcd/pkg/runtime v0.27.0
+	github.com/fluxcd/pkg/runtime v0.28.0
 	github.com/fluxcd/pkg/sourceignore v0.3.0
 	github.com/fluxcd/pkg/ssh v0.7.0
 	github.com/fluxcd/pkg/testserver v0.4.0

go.sum

@@ -541,8 +541,8 @@ github.com/fluxcd/pkg/masktoken v0.2.0 h1:HoSPTk4l1fz5Fevs2vVRvZGru33blfMwWSZKsH
 github.com/fluxcd/pkg/masktoken v0.2.0/go.mod h1:EA7GleAHL33kN6kTW06m5R3/Q26IyuGO7Ef/0CtpDI0=
 github.com/fluxcd/pkg/oci v0.18.0 h1:x5n3gW1lX6wrqvWP4ZkOXJ8LqLKy891uKwifCXSqKi4=
 github.com/fluxcd/pkg/oci v0.18.0/go.mod h1:zXoxvE4uuIEOgA98IM5Wv/uRxs7sdbaTlGDjzHb9yiA=
-github.com/fluxcd/pkg/runtime v0.27.0 h1:zVA95Z0KvNjvZxEZhvIbJyJIwtaiv1aVttHZ4YB/FzY=
-github.com/fluxcd/pkg/runtime v0.27.0/go.mod h1:fC1l4Wv1hnsqPKB46eDZBXF8RMZm5FXeU4bnJkwGkqk=
+github.com/fluxcd/pkg/runtime v0.28.0 h1:FtdZk53oMFUKIGykDtWNi3Pv2lXR6NHPWNqLQV5rpPg=
+github.com/fluxcd/pkg/runtime v0.28.0/go.mod h1:fC1l4Wv1hnsqPKB46eDZBXF8RMZm5FXeU4bnJkwGkqk=
 github.com/fluxcd/pkg/sourceignore v0.3.0 h1:pFO3hKV9ub+2SrNZPZE7xfiRhxsycRrd7JK7qB26nVw=
 github.com/fluxcd/pkg/sourceignore v0.3.0/go.mod h1:ak3Tve/KwVzytZ5V2yBlGGpTJ/2oQ9kcP3iuwBOAHGo=
 github.com/fluxcd/pkg/ssh v0.7.0 h1:FX5ky8SU9dYwbM6zEIDR3TSveLF01iyS95CtB5Ykpno=

internal/reconcile/summarize/summary_test.go

@@ -371,7 +371,7 @@ func TestSummarizeAndPatch(t *testing.T) {
 			// Check if the object status is valid as per kstatus.
 			condns := &conditionscheck.Conditions{NegativePolarity: testReadyConditions.NegativePolarity}
 			checker := conditionscheck.NewChecker(client, condns)
-			checker.CheckErr(ctx, obj)
+			checker.WithT(g).CheckErr(ctx, obj)
 		})
 	}
 }