Merge pull request #832 from souleb/issue-830

Fix Panic when no artifact in source

Author: stefanprodan

controllers/helmchart_controller.go

@@ -392,7 +392,8 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
 
 	// Assert source has an artifact
 	if s.GetArtifact() == nil || !r.Storage.ArtifactExist(*s.GetArtifact()) {
-		if helmRepo, ok := s.(*sourcev1.HelmRepository); !ok || !helmreg.IsOCI(helmRepo.Spec.URL) {
+		// Set the condition to indicate that the source has no artifact for all types except OCI HelmRepository
+		if helmRepo, ok := s.(*sourcev1.HelmRepository); !ok || helmRepo.Spec.Type != sourcev1.HelmRepositoryTypeOCI {
 			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, "NoSourceArtifact",
 				"no artifact available for %s source '%s'", obj.Spec.SourceRef.Kind, obj.Spec.SourceRef.Name)
 			r.eventLogf(ctx, obj, events.EventTypeTrace, "NoSourceArtifact",

controllers/helmchart_controller_test.go

@@ -515,6 +515,156 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
 	}
 }
 
+func TestHelmChartReconciler_reconcileFromHelmRepository(t *testing.T) {
+	g := NewWithT(t)
+
+	const (
+		chartName          = "helmchart"
+		chartVersion       = "0.2.0"
+		higherChartVersion = "0.3.0"
+		chartPath          = "testdata/charts/helmchart"
+	)
+
+	serverFactory, err := helmtestserver.NewTempHelmServer()
+	g.Expect(err).NotTo(HaveOccurred())
+	defer os.RemoveAll(serverFactory.Root())
+
+	for _, ver := range []string{chartVersion, higherChartVersion} {
+		g.Expect(serverFactory.PackageChartWithVersion(chartPath, ver)).To(Succeed())
+	}
+	g.Expect(serverFactory.GenerateIndex()).To(Succeed())
+
+	tests := []struct {
+		name       string
+		beforeFunc func(repository *sourcev1.HelmRepository)
+		assertFunc func(g *WithT, obj *sourcev1.HelmChart)
+	}{
+		{
+			name: "Reconciles chart build",
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart) {
+				key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
+				// Wait for HelmChart to be Ready
+				g.Eventually(func() bool {
+					if err := testEnv.Get(ctx, key, obj); err != nil {
+						return false
+					}
+					if !conditions.IsReady(obj) || obj.Status.Artifact == nil {
+						return false
+					}
+					readyCondition := conditions.Get(obj, meta.ReadyCondition)
+					return obj.Generation == readyCondition.ObservedGeneration &&
+						obj.Generation == obj.Status.ObservedGeneration
+				}, timeout).Should(BeTrue())
+
+				// Check if the object status is valid.
+				condns := &status.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
+				checker := status.NewChecker(testEnv.Client, condns)
+				checker.CheckErr(ctx, obj)
+			},
+		},
+		{
+			name: "Stalling on invalid repository URL",
+			beforeFunc: func(repository *sourcev1.HelmRepository) {
+				repository.Spec.URL = "://unsupported" // Invalid URL
+			},
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart) {
+				key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
+				// Wait for HelmChart to be FetchFailed == true
+				g.Eventually(func() bool {
+					if err := testEnv.Get(ctx, key, obj); err != nil {
+						return false
+					}
+					if !conditions.IsTrue(obj, sourcev1.FetchFailedCondition) {
+						return false
+					}
+					// observedGeneration is -1 because we have no successful reconciliation
+					return obj.Status.ObservedGeneration == -1
+				}, timeout).Should(BeTrue())
+
+				// Check if the object status is valid.
+				condns := &status.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
+				checker := status.NewChecker(testEnv.Client, condns)
+				checker.CheckErr(ctx, obj)
+			},
+		},
+		{
+			name: "Stalling on invalid oci repository URL",
+			beforeFunc: func(repository *sourcev1.HelmRepository) {
+				repository.Spec.URL = strings.Replace(repository.Spec.URL, "http", "oci", 1)
+			},
+			assertFunc: func(g *WithT, obj *sourcev1.HelmChart) {
+				key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
+				// Wait for HelmChart to be Ready
+				g.Eventually(func() bool {
+					if err := testEnv.Get(ctx, key, obj); err != nil {
+						return false
+					}
+					if !conditions.IsTrue(obj, sourcev1.FetchFailedCondition) {
+						return false
+					}
+					// observedGeneration is -1 because we have no successful reconciliation
+					return obj.Status.ObservedGeneration == -1
+				}, timeout).Should(BeTrue())
+
+				// Check if the object status is valid.
+				condns := &status.Conditions{NegativePolarity: helmChartReadyCondition.NegativePolarity}
+				checker := status.NewChecker(testEnv.Client, condns)
+				checker.CheckErr(ctx, obj)
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			server := testserver.NewHTTPServer(serverFactory.Root())
+			server.Start()
+			defer server.Stop()
+
+			ns, err := testEnv.CreateNamespace(ctx, "helmchart")
+			g.Expect(err).ToNot(HaveOccurred())
+			defer func() { g.Expect(testEnv.Delete(ctx, ns)).To(Succeed()) }()
+
+			repository := sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "helmrepository-",
+					Namespace:    ns.Name,
+				},
+				Spec: sourcev1.HelmRepositorySpec{
+					URL: server.URL(),
+				},
+			}
+
+			if tt.beforeFunc != nil {
+				tt.beforeFunc(&repository)
+			}
+
+			g.Expect(testEnv.CreateAndWait(ctx, &repository)).To(Succeed())
+
+			obj := sourcev1.HelmChart{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "helmrepository-reconcile-",
+					Namespace:    ns.Name,
+				},
+				Spec: sourcev1.HelmChartSpec{
+					Chart:   chartName,
+					Version: chartVersion,
+					SourceRef: sourcev1.LocalHelmChartSourceReference{
+						Kind: sourcev1.HelmRepositoryKind,
+						Name: repository.Name,
+					},
+				},
+			}
+			g.Expect(testEnv.Create(ctx, &obj)).To(Succeed())
+
+			if tt.assertFunc != nil {
+				tt.assertFunc(g, &obj)
+			}
+		})
+	}
+}
+
 func TestHelmChartReconciler_buildFromHelmRepository(t *testing.T) {
 	g := NewWithT(t)