Address 'update profile' review

pkg/helm/releases.go

@@ -44,17 +44,6 @@ func MakeHelmRelease(name, version, cluster, namespace string, helmRepository ty
 	}
 }
 
-// FindReleaseInNamespace iterates through a slice of HelmReleases to find one with a given name in a given namespace, and returns it with its index.
-func FindReleaseInNamespace(existingReleases []helmv2beta1.HelmRelease, name, ns string) (*helmv2beta1.HelmRelease, int, error) {
-	for i, r := range existingReleases {
-		if r.Name == name && r.Namespace == ns {
-			return &r, i, nil
-		}
-	}
-
-	return nil, -1, nil
-}
-
 // AppendHelmReleaseToString appends a HelmRelease to a string.
 func AppendHelmReleaseToString(content string, newRelease *helmv2beta1.HelmRelease) (string, error) {
 	var sb strings.Builder
@@ -73,8 +62,8 @@ func AppendHelmReleaseToString(content string, newRelease *helmv2beta1.HelmRelea
 }
 
 // SplitHelmReleaseYAML splits a manifest file that contains one or more Helm Releases that may be separated by '---'.
-func SplitHelmReleaseYAML(resources []byte) ([]helmv2beta1.HelmRelease, error) {
-	var helmReleaseList []helmv2beta1.HelmRelease
+func SplitHelmReleaseYAML(resources []byte) ([]*helmv2beta1.HelmRelease, error) {
+	var helmReleaseList []*helmv2beta1.HelmRelease
 
 	decoder := apimachinery.NewYAMLOrJSONDecoder(bytes.NewReader(resources), 100000000)
 
@@ -88,15 +77,14 @@ func SplitHelmReleaseYAML(resources []byte) ([]helmv2beta1.HelmRelease, error) {
 			return nil, err
 		}
 
-		helmReleaseList = append(helmReleaseList, value)
+		helmReleaseList = append(helmReleaseList, &value)
 	}
 
 	return helmReleaseList, nil
 }
 
-func PatchHelmRelease(existingReleases []helmv2beta1.HelmRelease, patchedHelmRelease helmv2beta1.HelmRelease, index int) (string, error) {
-	existingReleases[index] = patchedHelmRelease
-
+// MarshalHelmRelease marshals a list of HelmReleases.
+func MarshalHelmRelease(existingReleases []*helmv2beta1.HelmRelease) (string, error) {
 	var sb strings.Builder
 
 	for _, r := range existingReleases {

pkg/helm/releases_test.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/weaveworks/weave-gitops/pkg/helm"
 
-	"github.com/fluxcd/helm-controller/api/v2beta1"
 	helmv2beta1 "github.com/fluxcd/helm-controller/api/v2beta1"
 	sourcev1beta1 "github.com/fluxcd/source-controller/api/v1beta1"
 	"github.com/google/go-cmp/cmp"
@@ -86,57 +85,51 @@ var _ = Describe("AppendHelmReleaseToString", func() {
 	})
 })
 
-var _ = Describe("FindReleaseInNamespace", func() {
-	var (
-		name = "prod-podinfo"
-		ns   = "weave-system"
-	)
+var _ = Describe("MarshalHelmRelease", func() {
+	It("returns a string with an updated list of HelmReleases", func() {
+		release1 := helm.MakeHelmRelease(
+			"random-profile", "7.0.0", "prod", "weave-system",
+			types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"},
+		)
+		releaseBytes1, _ := kyaml.Marshal(release1)
+
+		release2 := helm.MakeHelmRelease(
+			"podinfo", "6.0.0", "prod", "weave-system",
+			types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"},
+		)
+		releaseBytes2, _ := kyaml.Marshal(release2)
 
-	When("it finds a HelmRelease with a matching name and namespace", func() {
-		It("returns its index in the slice of bytes", func() {
-			newRelease := helm.MakeHelmRelease(
+		patchedContent, err := helm.MarshalHelmRelease([]*helmv2beta1.HelmRelease{release1, release2})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(cmp.Diff(patchedContent, "---\n"+string(releaseBytes1)+"---\n"+string(releaseBytes2))).To(BeEmpty())
+	})
+})
+
+var _ = Describe("SplitHelmReleaseYAML", func() {
+	When("the resource contains only HelmRelease", func() {
+		It("returns a slice of HelmReleases", func() {
+			r1 := helm.MakeHelmRelease(
 				"podinfo", "6.0.0", "prod", "weave-system",
 				types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"},
 			)
-			existingRelease, index, err := helm.FindReleaseInNamespace([]helmv2beta1.HelmRelease{*newRelease}, name, ns)
+			r2 := helm.MakeHelmRelease(
+				"profile", "6.0.1", "prod", "test-namespace",
+				types.NamespacedName{Name: "helm-repo-name", Namespace: "test-namespace"},
+			)
+			b1, _ := kyaml.Marshal(r1)
+			bytes := append(b1, []byte("\n---\n")...)
+			b2, _ := kyaml.Marshal(r2)
+			list, err := helm.SplitHelmReleaseYAML(append(bytes, b2...))
 			Expect(err).NotTo(HaveOccurred())
-			Expect(index).To(Equal(0))
-			Expect(cmp.Diff(*existingRelease, *newRelease)).To(BeEmpty())
+			Expect(list).To(ContainElements(r1, r2))
 		})
 	})
 
-	When("it does not find a HelmRelease with a matching name and namespace", func() {
-		It("returns an index of -1", func() {
-			_, index, err := helm.FindReleaseInNamespace([]helmv2beta1.HelmRelease{}, name, ns)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(index).To(Equal(-1))
+	When("the resource contains any resource other than a HelmRelease", func() {
+		It("returns an error", func() {
+			b, _ := kyaml.Marshal("content")
+			_, err := helm.SplitHelmReleaseYAML(b)
+			Expect(err).To(MatchError("error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v2beta1.HelmRelease"))
 		})
 	})
 })
-
-var _ = Describe("PatchHelmReleaseInString", func() {
-	var (
-		r *v2beta1.HelmRelease
-	)
-
-	BeforeEach(func() {
-		r = helm.MakeHelmRelease(
-			"podinfo", "6.0.1", "prod", "weave-system",
-			types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"},
-		)
-	})
-
-	It("returns a string with an updated list of HelmReleases", func() {
-		existingRelease := helm.MakeHelmRelease(
-			"podinfo", "6.0.0", "prod", "weave-system",
-			types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"},
-		)
-
-		expectedContentBytes, _ := kyaml.Marshal(r)
-		expectedContent := "---\n" + string(expectedContentBytes)
-
-		patchedContent, err := helm.PatchHelmRelease([]helmv2beta1.HelmRelease{*existingRelease}, *r, 0)
-		Expect(err).NotTo(HaveOccurred())
-		Expect(cmp.Diff(patchedContent, expectedContent)).To(BeEmpty())
-	})
-})

pkg/services/profiles/add.go

@@ -10,6 +10,7 @@ import (
 	"github.com/weaveworks/weave-gitops/pkg/models"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
+	helmv2beta1 "github.com/fluxcd/helm-controller/api/v2beta1"
 	"github.com/google/uuid"
 	"k8s.io/apimachinery/pkg/types"
 )
@@ -120,12 +121,19 @@ func addHelmRelease(helmRepo types.NamespacedName, fileContent, name, version, c
 
 	newRelease := helm.MakeHelmRelease(name, version, cluster, ns, helmRepo)
 
-	matchingHelmRelease, _, err := helm.FindReleaseInNamespace(existingReleases, newRelease.Name, ns)
-	if matchingHelmRelease != nil {
+	if releaseIsInNamespace(existingReleases, newRelease.Name, ns) {
 		return "", fmt.Errorf("found another HelmRelease for profile '%s' in namespace %s", name, ns)
-	} else if err != nil {
-		return "", fmt.Errorf("error reading from %s: %w", models.WegoProfilesPath, err)
 	}
 
 	return helm.AppendHelmReleaseToString(fileContent, newRelease)
 }
+
+func releaseIsInNamespace(existingReleases []*helmv2beta1.HelmRelease, name, ns string) bool {
+	for _, r := range existingReleases {
+		if r.Name == name && r.Namespace == ns {
+			return true
+		}
+	}
+
+	return false
+}

pkg/services/profiles/update.go

@@ -10,6 +10,7 @@ import (
 	"github.com/weaveworks/weave-gitops/pkg/models"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
+	helmv2beta1 "github.com/fluxcd/helm-controller/api/v2beta1"
 	"github.com/google/uuid"
 	"k8s.io/apimachinery/pkg/types"
 )
@@ -120,20 +121,26 @@ func updateHelmRelease(helmRepo types.NamespacedName, fileContent, name, version
 		return "", fmt.Errorf("error splitting into YAML: %w", err)
 	}
 
-	releaseName := cluster + "-" + name
-
-	matchingHelmRelease, index, err := helm.FindReleaseInNamespace(existingReleases, releaseName, ns)
-	if matchingHelmRelease == nil {
-		return "", fmt.Errorf("failed to find HelmRelease '%s' in namespace %s", releaseName, ns)
-	} else if err != nil {
-		return "", fmt.Errorf("error reading from %s: %w", models.WegoProfilesPath, err)
+	updatedReleases, err := patchRelease(existingReleases, cluster+"-"+name, ns, version)
+	if err != nil {
+		return "", err
 	}
 
-	if matchingHelmRelease.Spec.Chart.Spec.Version == version {
-		return "", fmt.Errorf("version %s of profile '%s' already installed in %s/%s", version, name, ns, cluster)
-	}
+	return helm.MarshalHelmRelease(updatedReleases)
+}
 
-	matchingHelmRelease.Spec.Chart.Spec.Version = version
+func patchRelease(existingReleases []*helmv2beta1.HelmRelease, name, ns, version string) ([]*helmv2beta1.HelmRelease, error) {
+	for _, r := range existingReleases {
+		if r.Name == name && r.Namespace == ns {
+			if r.Spec.Chart.Spec.Version == version {
+				return nil, fmt.Errorf("version %s of HelmRelease '%s' already installed in namespace '%s'", version, name, ns)
+			}
+
+			r.Spec.Chart.Spec.Version = version
+
+			return existingReleases, nil
+		}
+	}
 
-	return helm.PatchHelmRelease(existingReleases, *matchingHelmRelease, index)
+	return nil, fmt.Errorf("failed to find HelmRelease '%s' in namespace %s", name, ns)
 }

pkg/services/profiles/update_test.go

@@ -147,7 +147,7 @@ var _ = Describe("Update Profile(s)", func() {
 						})
 					})
 
-					When("an existing HelmRelease is the same version as the want to update to", func() {
+					When("an existing HelmRelease is the same version as the one to update to", func() {
 						It("returns an error", func() {
 							existingRelease := helm.MakeHelmRelease(
 								"podinfo", "6.0.1", "prod", "weave-system",
@@ -162,7 +162,7 @@ var _ = Describe("Update Profile(s)", func() {
 							}}, nil)
 
 							err := profilesSvc.Update(context.TODO(), gitProviders, updateOptions)
-							Expect(err).To(MatchError("failed to update HelmRelease for profile 'podinfo' in profiles.yaml: version 6.0.1 of profile 'podinfo' already installed in weave-system/prod"))
+							Expect(err).To(MatchError("failed to update HelmRelease for profile 'podinfo' in profiles.yaml: version 6.0.1 of HelmRelease 'prod-podinfo' already installed in namespace 'weave-system'"))
 						})
 					})
 				})
@@ -182,7 +182,7 @@ var _ = Describe("Update Profile(s)", func() {
 						}}, nil)
 
 						err := profilesSvc.Update(context.TODO(), gitProviders, updateOptions)
-						Expect(err).To(MatchError("failed to update HelmRelease for profile 'podinfo' in profiles.yaml: failed to find HelmRelease 'prod-podinfo' in namespace weave-system"))
+						Expect(err).To(MatchError("failed to update HelmRelease for profile 'podinfo' in profiles.yaml: failed to find HelmRelease 'prod-podinfo' in namespace 'weave-system'"))
 					})
 				})
 

test/acceptance/test/profiles_test.go

@@ -131,7 +131,7 @@ Namespace: %s`, clusterName, namespace)))
 		}, "120s", "1s").Should(Equal(http.StatusOK))
 
 		By("Updating the version of the installed profile")
-		time.Sleep(4 * time.Minute)
+		time.Sleep(3 * time.Minute)
 		Eventually(func() string {
 			stdOut, stdErr = runCommandAndReturnStringOutput(fmt.Sprintf("%s update profile --name %s --version 6.0.0 --namespace %s --cluster %s --config-repo %s --auto-merge", gitopsBinaryPath, profileName, namespace, clusterName, appRepoRemoteURL))
 			return stdErr