Set Ready on OCI HelmRepository objects

api/v1/helmrepository_types.go

@@ -156,6 +156,10 @@ const (
 	// IndexationFailedReason signals that the HelmRepository index fetch
 	// failed.
 	IndexationFailedReason string = "IndexationFailed"
+
+	// NoIndexReason signals that the HelmRepository is of type OCI and does
+	// not produce an index Artifact. Charts are resolved on demand.
+	NoIndexReason string = "NoIndex"
 )
 
 // GetConditions returns the status conditions of the object.

docs/spec/v1/helmrepositories.md

@@ -131,12 +131,12 @@ You can run this example by saving the manifest into `helmrepository.yaml`.
 
    ```console
    NAME      URL                                 AGE     READY   STATUS
-   podinfo   oci://ghcr.io/stefanprodan/charts   3m22s
+   podinfo   oci://ghcr.io/stefanprodan/charts   3m22s   True    OCI HelmRepositories do not produce an index artifact; charts are resolved on demand
    ```
 
-Because the OCI Helm repository is a data container, there's nothing to report
-for `READY` and `STATUS` columns above. The existence of the object can be
-considered to be ready for use.
+The controller sets `Ready=True` with reason `NoIndex`. OCI Helm repositories
+do not produce an index Artifact; [HelmCharts](helmcharts.md) resolve charts
+from the registry on demand.
 
 ## Writing a HelmRepository spec
 
@@ -522,10 +522,11 @@ For practical information, see
 
 ## Working with HelmRepositories
 
-**Note:** This section does not apply to [OCI Helm
-Repositories](#helm-oci-repository), being a data container, once created, they
-are ready to used by [HelmCharts](helmcharts.md).
-
+**Note:** Triggering a reconcile and fetching an index Artifact do not apply to
+[OCI Helm Repositories](#helm-oci-repository). They are static data containers.
+Waiting for `Ready` does apply: the controller sets `Ready=True` with reason
+`NoIndex` after the object is recorded as a static source.
+
 ### Triggering a reconcile
 
 To manually tell the source-controller to reconcile a HelmRepository outside the
@@ -626,9 +627,10 @@ flux resume source helm <repository-name>
 
 ### Debugging a HelmRepository
 
-**Note:** This section does not apply to [OCI Helm
-Repositories](#helm-oci-repository), being a data container, they are static
-objects that don't require debugging if valid.
+**Note:** [OCI Helm Repositories](#helm-oci-repository) do not fetch an index.
+Chart pull failures are reported on the [HelmChart](helmcharts.md). The
+HelmRepository itself reports `Ready=True` with reason `NoIndex` when it has
+been recorded as a static source.
 
 There are several ways to gather information about a HelmRepository for debugging
 purposes.
@@ -695,9 +697,9 @@ specific HelmRepository, e.g. `flux logs --level=error --kind=HelmRepository --n
 
 ## HelmRepository Status
 
-**Note:** This section does not apply to [OCI Helm
-Repositories](#helm-oci-repository), they do not contain any information in the
-status.
+**Note:** [OCI Helm Repositories](#helm-oci-repository) do not report an
+Artifact. They set a `Ready=True` condition with reason `NoIndex`. The rest of
+this section describes HTTP/S Helm repositories.
 
 ### Artifact
 
@@ -782,13 +784,23 @@ characteristics:
 - The revision of the reported Artifact is up-to-date with the latest
   revision of the Helm repository.
 
+[OCI Helm Repositories](#helm-oci-repository) are marked ready without an
+Artifact. Charts are resolved on demand by [HelmChart](helmcharts.md).
+
 When the HelmRepository is "ready", the controller sets a Condition with the following
 attributes in the HelmRepository's `.status.conditions`:
 
 - `type: Ready`
 - `status: "True"`
 - `reason: Succeeded`
 
+For [OCI Helm Repositories](#helm-oci-repository), there is no Artifact. After
+the object is recorded as a static source, the controller sets:
+
+- `type: Ready`
+- `status: "True"`
+- `reason: NoIndex`
+
 This `Ready` Condition will retain a status value of `"True"` until the
 HelmRepository is marked as [reconciling](#reconciling-helmrepository), or e.g.
 a [transient error](#failed-helmrepository) occurs due to a temporary network

internal/controller/helmrepository_controller.go

@@ -696,6 +696,8 @@ func (r *HelmRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Ob
 	r.Eventf(obj, eventType, reason, "%s", msg)
 }
 
+const ociHelmRepositoryNoIndexMessage = "OCI HelmRepositories do not produce an index artifact; charts are resolved on demand"
+
 // migrateToStatic is HelmRepository OCI migration to static object.
 func (r *HelmRepositoryReconciler) migrationToStatic(ctx context.Context, sp *patch.SerialPatcher, obj *sourcev1.HelmRepository) (result ctrl.Result, err error) {
 	// Skip migration if suspended and not being deleted.
@@ -708,14 +710,22 @@ func (r *HelmRepositoryReconciler) migrationToStatic(ctx context.Context, sp *pa
 		return ctrl.Result{}, nil
 	}
 
-	// Delete any artifact.
+	// Delete any leftover HTTP-style artifact.
 	_, err = r.reconcileDelete(ctx, obj)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	// Delete finalizer and reset the status.
+	// Remove the source finalizer; OCI objects are static data containers.
 	controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer)
-	obj.Status = sourcev1.HelmRepositoryStatus{}
+
+	if obj.DeletionTimestamp.IsZero() {
+		obj.Status = sourcev1.HelmRepositoryStatus{
+			ObservedGeneration: obj.GetGeneration(),
+		}
+		conditions.MarkTrue(obj, meta.ReadyCondition, sourcev1.NoIndexReason, "%s", ociHelmRepositoryNoIndexMessage)
+	} else {
+		obj.Status = sourcev1.HelmRepositoryStatus{}
+	}
 
 	if err := sp.Patch(ctx, obj); err != nil {
 		return ctrl.Result{}, err

internal/controller/helmrepository_controller_test.go

@@ -1652,10 +1652,17 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 		if err := testEnv.Get(ctx, key, obj); err != nil {
 			return false
 		}
+		ready := conditions.Get(obj, meta.ReadyCondition)
 		return newGen == obj.Generation &&
+			conditions.IsReady(obj) &&
+			ready != nil && ready.Reason == sourcev1.NoIndexReason &&
 			!intpredicates.HelmRepositoryOCIRequireMigration(obj)
 	}, timeout).Should(BeTrue())
 
+	g.Expect(obj.GetArtifact()).To(BeNil())
+	g.Expect(conditions.IsReady(obj)).To(BeTrue())
+	g.Expect(conditions.Get(obj, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+
 	g.Expect(testEnv.Delete(ctx, obj)).To(Succeed())
 
 	// Wait for HelmRepository to be deleted
@@ -1861,8 +1868,12 @@ func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {
 
 	g.Eventually(func() bool {
 		_ = testEnv.Get(ctx, hrKey, hr)
-		return !intpredicates.HelmRepositoryOCIRequireMigration(hr)
+		ready := conditions.Get(hr, meta.ReadyCondition)
+		return conditions.IsReady(hr) &&
+			ready != nil && ready.Reason == sourcev1.NoIndexReason &&
+			!intpredicates.HelmRepositoryOCIRequireMigration(hr)
 	}, timeout, time.Second).Should(BeTrue())
+	g.Expect(controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer)).To(BeFalse())
 
 	// Migrates updated object with finalizer.
 
@@ -1874,7 +1885,10 @@ func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {
 
 	g.Eventually(func() bool {
 		_ = testEnv.Get(ctx, hrKey, hr)
-		return !intpredicates.HelmRepositoryOCIRequireMigration(hr)
+		ready := conditions.Get(hr, meta.ReadyCondition)
+		return conditions.IsReady(hr) &&
+			ready != nil && ready.Reason == sourcev1.NoIndexReason &&
+			!intpredicates.HelmRepositoryOCIRequireMigration(hr)
 	}, timeout, time.Second).Should(BeTrue())
 
 	// Migrates deleted object with finalizer.
@@ -1917,3 +1931,218 @@ func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {
 		return false
 	}, timeout).Should(BeTrue())
 }
+
+func TestHelmRepositoryReconciler_ociReadyCondition(t *testing.T) {
+	g := NewWithT(t)
+
+	ns, err := testEnv.CreateNamespace(ctx, "hr-oci-ready-test")
+	g.Expect(err).ToNot(HaveOccurred())
+	t.Cleanup(func() {
+		g.Expect(testEnv.Cleanup(ctx, ns)).ToNot(HaveOccurred())
+	})
+
+	hr := &sourcev1.HelmRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: "hr-oci-",
+			Namespace:    ns.Name,
+		},
+		Spec: sourcev1.HelmRepositorySpec{
+			Type:     sourcev1.HelmRepositoryTypeOCI,
+			URL:      "oci://ghcr.io/stefanprodan/charts",
+			Interval: metav1.Duration{Duration: interval},
+		},
+	}
+	g.Expect(testEnv.Create(ctx, hr)).ToNot(HaveOccurred())
+
+	g.Eventually(func() bool {
+		if err := testEnv.Get(ctx, client.ObjectKeyFromObject(hr), hr); err != nil {
+			return false
+		}
+		ready := conditions.Get(hr, meta.ReadyCondition)
+		return conditions.IsReady(hr) &&
+			ready != nil && ready.Reason == sourcev1.NoIndexReason &&
+			hr.GetArtifact() == nil &&
+			!intpredicates.HelmRepositoryOCIRequireMigration(hr)
+	}, timeout).Should(BeTrue())
+
+	g.Expect(hr.GetArtifact()).To(BeNil())
+	g.Expect(hr.Status.URL).To(BeEmpty())
+	g.Expect(controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer)).To(BeFalse())
+	g.Expect(conditions.Get(hr, meta.ReadyCondition).Message).To(ContainSubstring("do not produce an index artifact"))
+
+	// Spec updates on a static Ready object must not wipe the condition.
+	patchHelper, err := patch.NewHelper(hr, testEnv.Client)
+	g.Expect(err).ToNot(HaveOccurred())
+	hr.Spec.URL = "oci://ghcr.io/stefanprodan/charts/podinfo"
+	g.Expect(patchHelper.Patch(ctx, hr)).ToNot(HaveOccurred())
+
+	g.Consistently(func() bool {
+		if err := testEnv.Get(ctx, client.ObjectKeyFromObject(hr), hr); err != nil {
+			return false
+		}
+		ready := conditions.Get(hr, meta.ReadyCondition)
+		return conditions.IsReady(hr) && ready != nil && ready.Reason == sourcev1.NoIndexReason && hr.GetArtifact() == nil
+	}, 2*time.Second, 200*time.Millisecond).Should(BeTrue())
+}
+
+func TestHelmRepositoryReconciler_migrationToStatic(t *testing.T) {
+	g := NewWithT(t)
+
+	obj := &sourcev1.HelmRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "oci-repo",
+			Namespace:  "default",
+			Generation: 1,
+			Finalizers: []string{sourcev1.SourceFinalizer},
+		},
+		Spec: sourcev1.HelmRepositorySpec{
+			Type:     sourcev1.HelmRepositoryTypeOCI,
+			URL:      "oci://example.com/charts",
+			Interval: metav1.Duration{Duration: interval},
+		},
+		Status: sourcev1.HelmRepositoryStatus{
+			ObservedGeneration: 1,
+			URL:                "http://source-controller/index.yaml",
+			Artifact:           &meta.Artifact{Path: "old-index.yaml"},
+		},
+	}
+	conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "fetched index")
+
+	r := &HelmRepositoryReconciler{
+		Client: fakeclient.NewClientBuilder().
+			WithScheme(testEnv.GetScheme()).
+			WithObjects(obj).
+			WithStatusSubresource(&sourcev1.HelmRepository{}).
+			Build(),
+		EventRecorder: record.NewFakeRecorder(32),
+		Storage:       testStorage,
+	}
+
+	_, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
+	g.Expect(err).ToNot(HaveOccurred())
+
+	got := &sourcev1.HelmRepository{}
+	g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got)).To(Succeed())
+	g.Expect(controllerutil.ContainsFinalizer(got, sourcev1.SourceFinalizer)).To(BeFalse())
+	g.Expect(got.GetArtifact()).To(BeNil())
+	g.Expect(got.Status.URL).To(BeEmpty())
+	g.Expect(conditions.IsReady(got)).To(BeTrue())
+	g.Expect(conditions.Get(got, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+	g.Expect(got.Status.ObservedGeneration).To(Equal(got.Generation))
+	g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(got)).To(BeFalse())
+
+	// A second reconcile must not wipe Ready.
+	_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
+	g.Expect(err).ToNot(HaveOccurred())
+	got2 := &sourcev1.HelmRepository{}
+	g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got2)).To(Succeed())
+	g.Expect(conditions.IsReady(got2)).To(BeTrue())
+	g.Expect(conditions.Get(got2, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+	g.Expect(got2.GetArtifact()).To(BeNil())
+}
+
+func TestHelmRepositoryReconciler_migrationToStatic_emptyStatus(t *testing.T) {
+	g := NewWithT(t)
+
+	obj := &sourcev1.HelmRepository{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "oci-empty",
+			Namespace:  "default",
+			Generation: 1,
+		},
+		Spec: sourcev1.HelmRepositorySpec{
+			Type:     sourcev1.HelmRepositoryTypeOCI,
+			URL:      "oci://example.com/charts",
+			Interval: metav1.Duration{Duration: interval},
+		},
+	}
+
+	r := &HelmRepositoryReconciler{
+		Client: fakeclient.NewClientBuilder().
+			WithScheme(testEnv.GetScheme()).
+			WithObjects(obj).
+			WithStatusSubresource(&sourcev1.HelmRepository{}).
+			Build(),
+		EventRecorder: record.NewFakeRecorder(32),
+		Storage:       testStorage,
+	}
+
+	_, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
+	g.Expect(err).ToNot(HaveOccurred())
+
+	got := &sourcev1.HelmRepository{}
+	g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got)).To(Succeed())
+	g.Expect(conditions.IsReady(got)).To(BeTrue())
+	g.Expect(conditions.Get(got, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+	g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(got)).To(BeFalse())
+
+	_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
+	g.Expect(err).ToNot(HaveOccurred())
+	got2 := &sourcev1.HelmRepository{}
+	g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got2)).To(Succeed())
+	g.Expect(conditions.IsReady(got2)).To(BeTrue())
+	g.Expect(conditions.Get(got2, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+}
+
+func TestHelmRepositoryReconciler_migrationToStatic_staleReady(t *testing.T) {
+	tests := []struct {
+		name      string
+		markReady func(obj *sourcev1.HelmRepository)
+	}{
+		{
+			name: "Succeeded reason",
+			markReady: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "fetched index")
+			},
+		},
+		{
+			name: "empty reason",
+			markReady: func(obj *sourcev1.HelmRepository) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, "", "")
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			obj := &sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "oci-stale-ready",
+					Namespace:  "default",
+					Generation: 1,
+				},
+				Spec: sourcev1.HelmRepositorySpec{
+					Type:     sourcev1.HelmRepositoryTypeOCI,
+					URL:      "oci://example.com/charts",
+					Interval: metav1.Duration{Duration: interval},
+				},
+				Status: sourcev1.HelmRepositoryStatus{
+					ObservedGeneration: 1,
+				},
+			}
+			tt.markReady(obj)
+			g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(obj)).To(BeTrue())
+
+			r := &HelmRepositoryReconciler{
+				Client: fakeclient.NewClientBuilder().
+					WithScheme(testEnv.GetScheme()).
+					WithObjects(obj).
+					WithStatusSubresource(&sourcev1.HelmRepository{}).
+					Build(),
+				EventRecorder: record.NewFakeRecorder(32),
+				Storage:       testStorage,
+			}
+
+			_, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
+			g.Expect(err).ToNot(HaveOccurred())
+
+			got := &sourcev1.HelmRepository{}
+			g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got)).To(Succeed())
+			g.Expect(conditions.IsReady(got)).To(BeTrue())
+			g.Expect(conditions.Get(got, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
+			g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(got)).To(BeFalse())
+		})
+	}
+}