From 011bfdd69a55a7a5a11f4bf7ec23c5b3ef45fb9a Mon Sep 17 00:00:00 2001 From: nikimanoledaki Date: Thu, 3 Feb 2022 10:49:25 +0100 Subject: [PATCH] Address Add Profile review comments part 2 --- pkg/services/automation/generator.go | 13 ----------- pkg/services/gitopswriter/gitopswriter.go | 2 +- pkg/services/profiles/add.go | 22 ++++++++++--------- pkg/services/profiles/add_test.go | 2 +- .../helm.toolkit.fluxcd.io_helmreleases.yaml | 2 +- 5 files changed, 15 insertions(+), 26 deletions(-) diff --git a/pkg/services/automation/generator.go b/pkg/services/automation/generator.go index 56d1fddf04..6e0f347ad1 100644 --- a/pkg/services/automation/generator.go +++ b/pkg/services/automation/generator.go @@ -5,7 +5,6 @@ import ( "context" "crypto/md5" "fmt" - "math/rand" "os" "path/filepath" "strings" @@ -411,18 +410,6 @@ func GenerateResourceName(url gitproviders.RepoURL) string { return models.ConstrainResourceName(url.RepositoryName()) } -// GetRandomString generates a random unique string and appends it to a given string. -func GetRandomString(s string) string { - data := "abcdefghijklmnopqrstuvwyz1234567890" - - b := make([]byte, 5) - for i := range b { - b[i] = data[rand.Intn(len(data))] - } - - return s + string(b) -} - func (rk ResourceKind) ToGVR() (schema.GroupVersionResource, error) { switch rk { case ResourceKindApplication: diff --git a/pkg/services/gitopswriter/gitopswriter.go b/pkg/services/gitopswriter/gitopswriter.go index fdc0656a51..f9d0108c48 100644 --- a/pkg/services/gitopswriter/gitopswriter.go +++ b/pkg/services/gitopswriter/gitopswriter.go @@ -102,7 +102,7 @@ func (dw *gitOpsDirectoryWriterSvc) AddApplication(ctx context.Context, app mode Description: fmt.Sprintf("Added yamls for %s", app.Name), CommitMessage: AddCommitMessage, TargetBranch: defaultBranch, - NewBranch: automation.GetRandomString("wego"), + NewBranch: automation.GetAppHash(app), Files: files, } diff --git a/pkg/services/profiles/add.go b/pkg/services/profiles/add.go index 24c6a93cbd..ced1a2582f 100644 --- a/pkg/services/profiles/add.go +++ b/pkg/services/profiles/add.go @@ -6,11 +6,11 @@ import ( "fmt" "io" + "github.com/google/uuid" "github.com/weaveworks/weave-gitops/pkg/git" "github.com/weaveworks/weave-gitops/pkg/gitproviders" "github.com/weaveworks/weave-gitops/pkg/helm" "github.com/weaveworks/weave-gitops/pkg/models" - "github.com/weaveworks/weave-gitops/pkg/services/automation" "k8s.io/apimachinery/pkg/types" "github.com/fluxcd/go-git-providers/gitprovider" @@ -35,19 +35,19 @@ type AddOptions struct { // Add installs an available profile in a cluster's namespace by appending a HelmRelease to the profile manifest in the config repo, // provided that such a HelmRelease does not exist with the same profile name and version in the same namespace and cluster. func (s *ProfilesSvc) Add(ctx context.Context, gitProvider gitproviders.GitProvider, opts AddOptions) error { - configRepoUrl, err := gitproviders.NewRepoURL(opts.ConfigRepo) + configRepoURL, err := gitproviders.NewRepoURL(opts.ConfigRepo) if err != nil { return fmt.Errorf("failed to parse url: %w", err) } - repoExists, err := gitProvider.RepositoryExists(ctx, configRepoUrl) + repoExists, err := gitProvider.RepositoryExists(ctx, configRepoURL) if err != nil { return fmt.Errorf("failed to check whether repository exists: %w", err) } else if !repoExists { - return fmt.Errorf("repository '%v' could not be found", configRepoUrl.String()) + return fmt.Errorf("repository %q could not be found", configRepoURL) } - defaultBranch, err := gitProvider.GetDefaultBranch(ctx, configRepoUrl) + defaultBranch, err := gitProvider.GetDefaultBranch(ctx, configRepoURL) if err != nil { return fmt.Errorf("failed to get default branch: %w", err) } @@ -74,9 +74,9 @@ func (s *ProfilesSvc) Add(ctx context.Context, gitProvider gitproviders.GitProvi newRelease := helm.MakeHelmRelease(opts.Name, version, opts.Cluster, opts.Namespace, helmRepo) - files, err := gitProvider.GetRepoDirFiles(ctx, configRepoUrl, git.GetSystemPath(opts.Cluster), defaultBranch) + files, err := gitProvider.GetRepoDirFiles(ctx, configRepoURL, git.GetSystemPath(opts.Cluster), defaultBranch) if err != nil { - return fmt.Errorf("failed to get files in '%s' for config repository '%s': %s", git.GetSystemPath(opts.Cluster), configRepoUrl.String(), err) + return fmt.Errorf("failed to get files in '%s' for config repository %q: %s", git.GetSystemPath(opts.Cluster), configRepoURL, err) } file, err := AppendProfileToFile(files, newRelease, git.GetProfilesPath(opts.Cluster, models.WegoProfilesPath)) @@ -84,12 +84,12 @@ func (s *ProfilesSvc) Add(ctx context.Context, gitProvider gitproviders.GitProvi return fmt.Errorf("failed to append HelmRelease to profiles file: %w", err) } - pr, err := gitProvider.CreatePullRequest(ctx, configRepoUrl, gitproviders.PullRequestInfo{ + pr, err := gitProvider.CreatePullRequest(ctx, configRepoURL, gitproviders.PullRequestInfo{ Title: fmt.Sprintf("GitOps add %s", opts.Name), Description: fmt.Sprintf("Add manifest for %s profile", opts.Name), CommitMessage: AddCommitMessage, TargetBranch: defaultBranch, - NewBranch: automation.GetRandomString("wego-"), + NewBranch: uuid.New().String(), Files: []gitprovider.CommitFile{file}, }) if err != nil { @@ -101,7 +101,7 @@ func (s *ProfilesSvc) Add(ctx context.Context, gitProvider gitproviders.GitProvi if opts.AutoMerge { s.Logger.Actionf("auto-merge=true; merging PR number %v", pr.Get().Number) - if err := gitProvider.MergePullRequest(ctx, configRepoUrl, pr.Get().Number, AddCommitMessage); err != nil { + if err := gitProvider.MergePullRequest(ctx, configRepoURL, pr.Get().Number, AddCommitMessage); err != nil { return fmt.Errorf("error auto-merging PR: %w", err) } } @@ -146,6 +146,8 @@ func AppendProfileToFile(files []*gitprovider.CommitFile, newRelease *v2beta1.He } content = *f.Content + + break } } diff --git a/pkg/services/profiles/add_test.go b/pkg/services/profiles/add_test.go index fbc0810833..318ea5b836 100644 --- a/pkg/services/profiles/add_test.go +++ b/pkg/services/profiles/add_test.go @@ -178,7 +178,7 @@ var _ = Describe("Add", func() { It("fails if the config repo does not exist", func() { gitProviders.RepositoryExistsReturns(false, nil) err := profilesSvc.Add(context.TODO(), gitProviders, addOptions) - Expect(err).To(MatchError("repository 'ssh://git@github.com/owner/config-repo.git' could not be found")) + Expect(err).To(MatchError("repository \"ssh://git@github.com/owner/config-repo.git\" could not be found")) Expect(gitProviders.RepositoryExistsCallCount()).To(Equal(1)) }) }) diff --git a/tools/testcrds/helm.toolkit.fluxcd.io_helmreleases.yaml b/tools/testcrds/helm.toolkit.fluxcd.io_helmreleases.yaml index 4a8270bf83..d71b468e8e 100644 --- a/tools/testcrds/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/tools/testcrds/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -432,7 +432,7 @@ spec: description: Optional marks this ValuesReference as optional. When set, a not found error for the values reference is ignored, but any ValuesKey, targetPath or transient error will still result in a reconciliation failure. type: boolean targetPath: - description: targetPath is the YAML dot notation path the value should be merged at. When set, the ValuesKey is expected to be a single flat value. Defaults to 'None', which results in the values getting merged at the root. + description: TargetPath is the YAML dot notation path the value should be merged at. When set, the ValuesKey is expected to be a single flat value. Defaults to 'None', which results in the values getting merged at the root. type: string valuesKey: description: ValuesKey is the data key where the values.yaml or a specific value can be found at. Defaults to 'values.yaml'.