From f7b90e858a2a9f204a42cbbf9a881f75f03c66dd Mon Sep 17 00:00:00 2001 From: Anders Qvist Date: Wed, 24 Nov 2021 16:31:49 +0100 Subject: [PATCH 1/4] Log all provider mutations. Signed-off-by: Anders Qvist --- pkg/command/promote.go | 2 ++ pkg/git/azdo.go | 8 ++++++++ pkg/git/github.go | 18 ++++++++++++++++-- pkg/git/repository.go | 6 +++++- tests/e2e_test.go | 10 +++++----- 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/pkg/command/promote.go b/pkg/command/promote.go index 5bd7b02..7a65844 100644 --- a/pkg/command/promote.go +++ b/pkg/command/promote.go @@ -3,6 +3,7 @@ package command import ( "context" "fmt" + "log" "github.com/fluxcd/image-automation-controller/pkg/update" imagev1alpha1_reflect "github.com/fluxcd/image-reflector-controller/api/v1alpha1" @@ -94,6 +95,7 @@ func updateImageTag(path, app, group, tag string) error { }, } + log.Printf("Updating images with %s:%s:%s in %s\n", group, app, tag, path) _, err := update.UpdateWithSetters(path, path, policies) if err != nil { return fmt.Errorf("failed updating manifests: %w", err) diff --git a/pkg/git/azdo.go b/pkg/git/azdo.go index b418c49..209eef5 100644 --- a/pkg/git/azdo.go +++ b/pkg/git/azdo.go @@ -3,6 +3,7 @@ package git import ( "context" "fmt" + "log" "net/url" "strings" "time" @@ -104,6 +105,9 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto GitPullRequestToUpdate: &updatePR, } _, err = g.client.UpdatePullRequest(ctx, updateArgs) + if err == nil { + log.Printf("Updated PR #%d merging %s -> %s\n", *pr.PullRequestId, sourceRefName, targetRefName) + } return err } @@ -126,6 +130,7 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto if err != nil { return err } + log.Printf("Created new PR #%d merging %s -> %s\n", *pr.PullRequestId, sourceRefName, targetRefName) if !auto { return nil } @@ -143,6 +148,9 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto GitPullRequestToUpdate: &updatePR, } _, err = g.client.UpdatePullRequest(ctx, updateArgs) + if err == nil { + log.Printf("Auto-merge activated for PR #%d\n", *pr.PullRequestId) + } return err } diff --git a/pkg/git/github.go b/pkg/git/github.go index 1354830..935c8a9 100644 --- a/pkg/git/github.go +++ b/pkg/git/github.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io/ioutil" + "log" "net/http" "os" "strings" @@ -101,12 +102,18 @@ func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, aut MaintainerCanModify: github.Bool(true), } pr, _, err = g.client.PullRequests.Create(ctx, g.owner, g.repo, createOpts) + if err == nil { + log.Printf("Created new PR #%d merging %s -> %s\n", pr.GetNumber(), sourceName, targetName) + } case 1: pr = (prsOnBranch)[0] pr.Title = &title pr.Body = &description pr.Base.Ref = &targetName pr, _, err = g.client.PullRequests.Edit(ctx, g.owner, g.repo, *pr.Number, pr) + if err == nil { + log.Printf("Updated PR #%d merging %s -> %s\n", pr.GetNumber(), sourceName, targetName) + } default: return fmt.Errorf("received more than one PRs when listing: %d", len(prsOnBranch)) } @@ -128,8 +135,14 @@ func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, aut PullRequestID: pr.GetNodeID(), } err = client.Mutate(ctx, &mutation, input, nil) - if err != nil && strings.Contains(err.Error(), "not in the correct state") { - err = g.MergePR(ctx, int(pr.GetNumber()), *pr.GetHead().SHA) + if err == nil { + log.Printf("Auto-merge activated for PR #%d\n", pr.GetNumber()) + } else { + log.Printf("Failed to activate auto-merge for PR %d: %v", pr.GetNumber(), err) + if strings.Contains(err.Error(), "not in the correct state") { + err = g.MergePR(ctx, int(pr.GetNumber()), *pr.GetHead().SHA) + log.Printf("Explicitly merged PR #%d\n", pr.GetNumber()) + } } } return err @@ -141,6 +154,7 @@ func (g *GitHubGITProvider) GetStatus(ctx context.Context, sha string, group str if err != nil { return Status{}, err } + log.Printf("Considering statuses %v\n", statuses) name := fmt.Sprintf("%s-%s", group, env) for _, s := range statuses { diff --git a/pkg/git/repository.go b/pkg/git/repository.go index 66d66ad..1e3f8e0 100644 --- a/pkg/git/repository.go +++ b/pkg/git/repository.go @@ -3,6 +3,7 @@ package git import ( "context" "fmt" + "log" "path/filepath" "time" @@ -131,10 +132,12 @@ func (g *Repository) CreateCommit(branchName, message string) (*git2go.Oid, erro if err != nil { return nil, err } - sha, err := g.gitRepository.CreateCommit(fmt.Sprintf("refs/heads/%s", branchName), signature, signature, message, tree, commitTarget) + refName := fmt.Sprintf("refs/heads/%s", branchName) + sha, err := g.gitRepository.CreateCommit(refName, signature, signature, message, tree, commitTarget) if err != nil { return nil, err } + log.Printf("Created commit %s on %s with message '%s'\n", sha, refName, message) return sha, nil } @@ -166,6 +169,7 @@ func (g *Repository) Push(branchName string, force bool) error { if err != nil { return fmt.Errorf("failed pushing branches %s: %w", branches, err) } + log.Printf("Pushed branch %s to remote\n", branches[0]) return nil } diff --git a/tests/e2e_test.go b/tests/e2e_test.go index 736c4b1..6a35a9d 100644 --- a/tests/e2e_test.go +++ b/tests/e2e_test.go @@ -97,7 +97,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, "created promotions pull request", newCommandMsgDev) + require.Contains(t, newCommandMsgDev, "created promotions pull request") path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, p.defaultBranch, group, "dev", app, tag) @@ -116,7 +116,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, "created promotions pull request", promoteCommandMsgQa) + require.Contains(t, promoteCommandMsgQa, "created promotions pull request") path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, promoteBranchName, group, "qa", app, tag) statusCommandMsgQa, err := testRunCommand( @@ -128,7 +128,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, "status check has succeed", statusCommandMsgQa) + require.Contains(t, statusCommandMsgQa, "status check has succeed") repoQa := testGetRepository(t, path) revQa := testGetRepositoryHeadRevision(t, repoQa) @@ -152,7 +152,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, "created promotions pull request", promoteCommandMsgProd) + require.Contains(t, promoteCommandMsgProd, "created promotions pull request") path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, promoteBranchName, group, "prod", app, tag) statusCommandMsgProd, err := testRunCommand( @@ -164,7 +164,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Equal(t, "status check has succeed", statusCommandMsgProd) + require.Contains(t, statusCommandMsgProd, "status check has succeed") repoProd := testGetRepository(t, path) revProd := testGetRepositoryHeadRevision(t, repoProd) From 345be8a17f8a064b806b71aad82bc7ded8e07762 Mon Sep 17 00:00:00 2001 From: Anders Qvist Date: Thu, 25 Nov 2021 22:53:49 +0100 Subject: [PATCH 2/4] Present PR number in message to user. --- pkg/command/promote.go | 4 ++-- pkg/git/azdo.go | 12 ++++++------ pkg/git/github.go | 12 ++++++------ pkg/git/github_test.go | 24 ++++++++++++++++++------ pkg/git/provider.go | 2 +- pkg/git/repository.go | 2 +- tests/e2e_test.go | 6 +++--- 7 files changed, 37 insertions(+), 25 deletions(-) diff --git a/pkg/command/promote.go b/pkg/command/promote.go index 7a65844..a703b3a 100644 --- a/pkg/command/promote.go +++ b/pkg/command/promote.go @@ -75,11 +75,11 @@ func promote(ctx context.Context, cfg config.Config, repo *git.Repository, state if err != nil { return "", fmt.Errorf("could not get environment automation state: %w", err) } - err = repo.CreatePR(ctx, state.BranchName(), auto, state) + prid, err := repo.CreatePR(ctx, state.BranchName(), auto, state) if err != nil { return "", fmt.Errorf("could not create a PR: %w", err) } - return "created promotions pull request", nil + return fmt.Sprintf("created branch %s with pull request %d", state.BranchName(), prid), nil } func updateImageTag(path, app, group, tag string) error { diff --git a/pkg/git/azdo.go b/pkg/git/azdo.go index 209eef5..49aab6f 100644 --- a/pkg/git/azdo.go +++ b/pkg/git/azdo.go @@ -68,13 +68,13 @@ func NewAzdoGITProvider(ctx context.Context, remoteURL, token string) (*AzdoGITP } // CreatePR ... -func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error { +func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) { sourceRefName := fmt.Sprintf("refs/heads/%s", branchName) targetRefName := fmt.Sprintf("refs/heads/%s", DefaultBranch) title := state.Title() description, err := state.Description() if err != nil { - return err + return 0, err } // Update PR if it already exists @@ -108,7 +108,7 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto if err == nil { log.Printf("Updated PR #%d merging %s -> %s\n", *pr.PullRequestId, sourceRefName, targetRefName) } - return err + return *pr.PullRequestId, err } // Create new PR @@ -128,11 +128,11 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto } pr, err := g.client.CreatePullRequest(ctx, createArgs) if err != nil { - return err + return 0, err } log.Printf("Created new PR #%d merging %s -> %s\n", *pr.PullRequestId, sourceRefName, targetRefName) if !auto { - return nil + return *pr.PullRequestId, nil } // This update is done to set auto merge. The reason this is not @@ -151,7 +151,7 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto if err == nil { log.Printf("Auto-merge activated for PR #%d\n", *pr.PullRequestId) } - return err + return *pr.PullRequestId, err } func (g *AzdoGITProvider) GetStatus(ctx context.Context, sha string, group string, env string) (Status, error) { diff --git a/pkg/git/github.go b/pkg/git/github.go index 935c8a9..9693b36 100644 --- a/pkg/git/github.go +++ b/pkg/git/github.go @@ -65,13 +65,13 @@ func NewGitHubGITProvider(ctx context.Context, remoteURL, token string) (*GitHub } // CreatePR ... -func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error { +func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) { sourceName := branchName targetName := DefaultBranch title := state.Title() description, err := state.Description() if err != nil { - return err + return 0, err } listOpts := &github.PullRequestListOptions{ @@ -81,7 +81,7 @@ func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, aut openPrs, _, err := g.client.PullRequests.List(ctx, g.owner, g.repo, listOpts) if err != nil { - return err + return 0, err } var prsOnBranch []*github.PullRequest @@ -115,11 +115,11 @@ func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, aut log.Printf("Updated PR #%d merging %s -> %s\n", pr.GetNumber(), sourceName, targetName) } default: - return fmt.Errorf("received more than one PRs when listing: %d", len(prsOnBranch)) + return 0, fmt.Errorf("received more than one PRs when listing: %d", len(prsOnBranch)) } if err != nil { - return err + return 0, err } if auto != (pr.GetAutoMerge() != nil) { @@ -145,7 +145,7 @@ func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, aut } } } - return err + return pr.GetNumber(), err } func (g *GitHubGITProvider) GetStatus(ctx context.Context, sha string, group string, env string) (Status, error) { diff --git a/pkg/git/github_test.go b/pkg/git/github_test.go index 222e0d0..f35d740 100644 --- a/pkg/git/github_test.go +++ b/pkg/git/github_test.go @@ -133,6 +133,7 @@ var _ = Describe("GitHubGITProvider CreatePR", func() { } }) + var prid int var err error var branchName string var auto bool @@ -145,7 +146,7 @@ var _ = Describe("GitHubGITProvider CreatePR", func() { } JustBeforeEach(func() { - err = provider.CreatePR(ctx, branchName, auto, state) + prid, err = provider.CreatePR(ctx, branchName, auto, state) }) When("Creating PR with empty values", func() { @@ -193,24 +194,34 @@ var _ = Describe("GitHubGITProvider CreatePR", func() { It("doesn't return an error", func() { Expect(err).To(BeNil()) }) + + It("returns the PR number", func() { + Expect(prid).To(BeNumerically(">", 0)) + }) }) When("Creating PR on a branch that already has a PR", func() { + var origPRId int var repo *Repository BeforeEach(func() { + var e error branchName = randomBranchName("testing-create-pr") repo = cloneTestRepoWithNewBranch(ctx, branchName) commitAFile(repo, branchName) pushBranch(repo, branchName) - e := provider.CreatePR(ctx, branchName, false, state) + origPRId, e = provider.CreatePR(ctx, branchName, false, state) Expect(e).To(BeNil()) }) It("doesn't return an error", func() { Expect(err).To(BeNil()) }) + + It("returns the original PRs id", func() { + Expect(prid).To(Equal(origPRId)) + }) }) When("Creating/updating a PR with automerge", func() { @@ -403,7 +414,7 @@ var _ = Describe("GitHubGITProvider MergePR", func() { repo2 := cloneTestRepoOnExistingBranch(ctx, branchName) state.Sha = commitAFile(repo2, branchName).String() pushBranch(repo2, branchName) - e := provider.CreatePR(ctx, branchName, false, state) + _, e := provider.CreatePR(ctx, branchName, false, state) Expect(e).To(BeNil()) pr, e := provider.GetPRWithBranch(ctx, branchName, DefaultBranch) @@ -455,6 +466,7 @@ var _ = Describe("GitHubGITProvider GetPRWithBranch", func() { }) When("Getting PR with existing branchName", func() { + var origPRId int BeforeEach(func() { branchName = randomBranchName("testing-get-pr-with-branch") @@ -465,13 +477,13 @@ var _ = Describe("GitHubGITProvider GetPRWithBranch", func() { state.Sha = commitAFile(repo2, branchName).String() pushBranch(repo2, branchName) - e = provider.CreatePR(ctx, branchName, false, state) + origPRId, e = provider.CreatePR(ctx, branchName, false, state) Expect(e).To(BeNil()) }) It("doesn't return an error and ID larger than 0", func() { Expect(err).To(BeNil()) - Expect(pr.ID).To(BeNumerically(">", 0)) + Expect(pr.ID).To(Equal(origPRId)) }) }) }) @@ -521,7 +533,7 @@ var _ = Describe("GitHubGITProvider GetPRThatCausedCommit", func() { commitSha := commitAFile(repo2, branchName) pushBranch(repo2, branchName) - e := provider.CreatePR(ctx, branchName, false, state) + _, e := provider.CreatePR(ctx, branchName, false, state) Expect(e).To(BeNil()) mergedPR, e = provider.GetPRWithBranch(ctx, branchName, DefaultBranch) diff --git a/pkg/git/provider.go b/pkg/git/provider.go index df44951..0a0a2bc 100644 --- a/pkg/git/provider.go +++ b/pkg/git/provider.go @@ -16,7 +16,7 @@ const ( type GitProvider interface { GetStatus(ctx context.Context, sha, group, env string) (Status, error) SetStatus(ctx context.Context, sha string, group string, env string, succeeded bool) error - CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error + CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) GetPRWithBranch(ctx context.Context, source, target string) (PullRequest, error) GetPRThatCausedCommit(ctx context.Context, sha string) (PullRequest, error) MergePR(ctx context.Context, ID int, sha string) error diff --git a/pkg/git/repository.go b/pkg/git/repository.go index 1e3f8e0..fdfdd27 100644 --- a/pkg/git/repository.go +++ b/pkg/git/repository.go @@ -173,7 +173,7 @@ func (g *Repository) Push(branchName string, force bool) error { return nil } -func (g *Repository) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error { +func (g *Repository) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) { return g.gitProvider.CreatePR(ctx, branchName, auto, state) } diff --git a/tests/e2e_test.go b/tests/e2e_test.go index 6a35a9d..9e18bca 100644 --- a/tests/e2e_test.go +++ b/tests/e2e_test.go @@ -97,7 +97,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Contains(t, newCommandMsgDev, "created promotions pull request") + require.Contains(t, newCommandMsgDev, fmt.Sprintf("created branch %s with pull request", promoteBranchName)) path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, p.defaultBranch, group, "dev", app, tag) @@ -116,7 +116,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Contains(t, promoteCommandMsgQa, "created promotions pull request") + require.Contains(t, promoteCommandMsgQa, fmt.Sprintf("created branch %s with pull request", promoteBranchName)) path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, promoteBranchName, group, "qa", app, tag) statusCommandMsgQa, err := testRunCommand( @@ -152,7 +152,7 @@ func TestProviderE2E(t *testing.T) { ) require.NoError(t, err) - require.Contains(t, promoteCommandMsgProd, "created promotions pull request") + require.Contains(t, promoteCommandMsgProd, fmt.Sprintf("created branch %s with pull request", promoteBranchName)) path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, promoteBranchName, group, "prod", app, tag) statusCommandMsgProd, err := testRunCommand( From 53c7bd626ddce8972eb4ee635f8b69a82646baf7 Mon Sep 17 00:00:00 2001 From: Anders Qvist Date: Thu, 25 Nov 2021 22:58:43 +0100 Subject: [PATCH 3/4] new/promote command says what commit it created. Signed-off-by: Anders Qvist --- pkg/command/promote.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/command/promote.go b/pkg/command/promote.go index a703b3a..1bac3ef 100644 --- a/pkg/command/promote.go +++ b/pkg/command/promote.go @@ -23,6 +23,8 @@ func PromoteCommand(ctx context.Context, providerType string, path, token string } pr, err := repo.GetPRThatCausedCurrentCommit(ctx) if err != nil { + sha, _ := repo.GetCurrentCommit() + log.Printf("Failed retrieving pull request for commit %s: %v", sha, err) //lint:ignore nilerr should not return error return "skipping PR creation as commit does not originate from promotion PR", nil } @@ -63,7 +65,7 @@ func promote(ctx context.Context, cfg config.Config, repo *git.Repository, state if err != nil { return "", fmt.Errorf("could not create branch: %w", err) } - _, err = repo.CreateCommit(state.BranchName(), state.Title()) + sha, err := repo.CreateCommit(state.BranchName(), state.Title()) if err != nil { return "", fmt.Errorf("could not commit changes: %w", err) } @@ -79,7 +81,7 @@ func promote(ctx context.Context, cfg config.Config, repo *git.Repository, state if err != nil { return "", fmt.Errorf("could not create a PR: %w", err) } - return fmt.Sprintf("created branch %s with pull request %d", state.BranchName(), prid), nil + return fmt.Sprintf("created branch %s with pull request %d on commit %s", state.BranchName(), prid, sha), nil } func updateImageTag(path, app, group, tag string) error { From 27e7f13281bbdefc9446c78219981116a6d6a075 Mon Sep 17 00:00:00 2001 From: Anders Qvist Date: Thu, 25 Nov 2021 23:58:06 +0100 Subject: [PATCH 4/4] Silence some lints temporarily; will be dealt with by other PR. Signed-off-by: Anders Qvist --- pkg/command/promote.go | 1 + pkg/git/azdo.go | 1 + pkg/git/github.go | 1 + 3 files changed, 3 insertions(+) diff --git a/pkg/command/promote.go b/pkg/command/promote.go index 1bac3ef..9b1b100 100644 --- a/pkg/command/promote.go +++ b/pkg/command/promote.go @@ -23,6 +23,7 @@ func PromoteCommand(ctx context.Context, providerType string, path, token string } pr, err := repo.GetPRThatCausedCurrentCommit(ctx) if err != nil { + //nolint:errcheck //best effort for logging sha, _ := repo.GetCurrentCommit() log.Printf("Failed retrieving pull request for commit %s: %v", sha, err) //lint:ignore nilerr should not return error diff --git a/pkg/git/azdo.go b/pkg/git/azdo.go index 49aab6f..e9bceed 100644 --- a/pkg/git/azdo.go +++ b/pkg/git/azdo.go @@ -68,6 +68,7 @@ func NewAzdoGITProvider(ctx context.Context, remoteURL, token string) (*AzdoGITP } // CreatePR ... +//nolint:funlen // temporary func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) { sourceRefName := fmt.Sprintf("refs/heads/%s", branchName) targetRefName := fmt.Sprintf("refs/heads/%s", DefaultBranch) diff --git a/pkg/git/github.go b/pkg/git/github.go index 9693b36..5ab0cc0 100644 --- a/pkg/git/github.go +++ b/pkg/git/github.go @@ -65,6 +65,7 @@ func NewGitHubGITProvider(ctx context.Context, remoteURL, token string) (*GitHub } // CreatePR ... +//nolint:gocognit //temporary func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) (int, error) { sourceName := branchName targetName := DefaultBranch