From 67be697153e6dfd59590e3a63cd04036d113b4e0 Mon Sep 17 00:00:00 2001 From: nikimanoledaki Date: Tue, 15 Feb 2022 17:09:04 +0100 Subject: [PATCH] Address 'update profile' review --- pkg/helm/releases.go | 21 ++++----- pkg/helm/releases_test.go | 68 +++++++++++++++++---------- pkg/services/profiles/add.go | 2 +- pkg/services/profiles/update.go | 4 +- pkg/services/profiles/update_test.go | 2 +- test/acceptance/test/profiles_test.go | 2 +- 6 files changed, 59 insertions(+), 40 deletions(-) diff --git a/pkg/helm/releases.go b/pkg/helm/releases.go index d78ad685a48..6e8f7609c19 100644 --- a/pkg/helm/releases.go +++ b/pkg/helm/releases.go @@ -44,15 +44,15 @@ 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 { +// FindReleaseInNamespace iterates through a slice of HelmReleases to find one with a given name in a given namespace. +func FindReleaseInNamespace(existingReleases []*helmv2beta1.HelmRelease, name, ns string) (*helmv2beta1.HelmRelease, error) { + for _, r := range existingReleases { if r.Name == name && r.Namespace == ns { - return &r, i, nil + return r, nil } } - return nil, -1, nil + return nil, nil } // AppendHelmReleaseToString appends a HelmRelease to a string. @@ -73,8 +73,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 +88,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 { diff --git a/pkg/helm/releases_test.go b/pkg/helm/releases_test.go index 1e4c3212fd0..388be958b6d 100644 --- a/pkg/helm/releases_test.go +++ b/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" @@ -93,50 +92,71 @@ var _ = Describe("FindReleaseInNamespace", func() { ) When("it finds a HelmRelease with a matching name and namespace", func() { - It("returns its index in the slice of bytes", func() { + It("returns the matching HelmRelease", func() { newRelease := helm.MakeHelmRelease( - "podinfo", "6.0.0", "prod", "weave-system", + "podinfo", "6.0.0", "prod", ns, types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"}, ) - existingRelease, index, err := helm.FindReleaseInNamespace([]helmv2beta1.HelmRelease{*newRelease}, name, ns) + existingRelease, err := helm.FindReleaseInNamespace([]*helmv2beta1.HelmRelease{newRelease}, name, ns) Expect(err).NotTo(HaveOccurred()) - Expect(index).To(Equal(0)) Expect(cmp.Diff(*existingRelease, *newRelease)).To(BeEmpty()) }) }) 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) + It("returns nil", func() { + r, err := helm.FindReleaseInNamespace([]*helmv2beta1.HelmRelease{}, name, ns) Expect(err).NotTo(HaveOccurred()) - Expect(index).To(Equal(-1)) + Expect(r).To(BeNil()) }) }) }) -var _ = Describe("PatchHelmReleaseInString", func() { - var ( - r *v2beta1.HelmRelease - ) - - BeforeEach(func() { - r = helm.MakeHelmRelease( - "podinfo", "6.0.1", "prod", "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) - It("returns a string with an updated list of HelmReleases", func() { - existingRelease := helm.MakeHelmRelease( + release2 := helm.MakeHelmRelease( "podinfo", "6.0.0", "prod", "weave-system", types.NamespacedName{Name: "helm-repo-name", Namespace: "helm-repo-namespace"}, ) + releaseBytes2, _ := kyaml.Marshal(release2) - expectedContentBytes, _ := kyaml.Marshal(r) - expectedContent := "---\n" + string(expectedContentBytes) - - patchedContent, err := helm.PatchHelmRelease([]helmv2beta1.HelmRelease{*existingRelease}, *r, 0) + patchedContent, err := helm.MarshalHelmRelease([]*helmv2beta1.HelmRelease{release1, release2}) Expect(err).NotTo(HaveOccurred()) - Expect(cmp.Diff(patchedContent, expectedContent)).To(BeEmpty()) + 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"}, + ) + 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(list).To(ContainElements(r1, r2)) + }) + }) + + 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")) + }) }) }) diff --git a/pkg/services/profiles/add.go b/pkg/services/profiles/add.go index 6ca5666b1da..362c15caa69 100644 --- a/pkg/services/profiles/add.go +++ b/pkg/services/profiles/add.go @@ -120,7 +120,7 @@ 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) + matchingHelmRelease, err := helm.FindReleaseInNamespace(existingReleases, newRelease.Name, ns) if matchingHelmRelease != nil { return "", fmt.Errorf("found another HelmRelease for profile '%s' in namespace %s", name, ns) } else if err != nil { diff --git a/pkg/services/profiles/update.go b/pkg/services/profiles/update.go index f877ab27320..23a392d0e56 100644 --- a/pkg/services/profiles/update.go +++ b/pkg/services/profiles/update.go @@ -122,7 +122,7 @@ func updateHelmRelease(helmRepo types.NamespacedName, fileContent, name, version releaseName := cluster + "-" + name - matchingHelmRelease, index, err := helm.FindReleaseInNamespace(existingReleases, releaseName, ns) + matchingHelmRelease, 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 { @@ -135,5 +135,5 @@ func updateHelmRelease(helmRepo types.NamespacedName, fileContent, name, version matchingHelmRelease.Spec.Chart.Spec.Version = version - return helm.PatchHelmRelease(existingReleases, *matchingHelmRelease, index) + return helm.MarshalHelmRelease(existingReleases) } diff --git a/pkg/services/profiles/update_test.go b/pkg/services/profiles/update_test.go index 3f39112f407..94e8008c659 100644 --- a/pkg/services/profiles/update_test.go +++ b/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", diff --git a/test/acceptance/test/profiles_test.go b/test/acceptance/test/profiles_test.go index aa216a33960..879f656befb 100644 --- a/test/acceptance/test/profiles_test.go +++ b/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