Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log mutations and present PR details to user #143

Merged
merged 4 commits into from
Nov 26, 2021
Merged

Log mutations and present PR details to user #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7b90e8
Log all provider mutations.
Nov 24, 2021
345be8a
Present PR number in message to user.
Nov 25, 2021
53c7bd6
new/promote command says what commit it created.
Nov 25, 2021
27e7f13
Silence some lints temporarily; will be dealt with by other PR.
Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions pkg/command/promote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -22,6 +23,9 @@ 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
return "skipping PR creation as commit does not originate from promotion PR", nil
}
Expand Down Expand Up @@ -62,7 +66,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)
}
Expand All @@ -74,11 +78,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 on commit %s", state.BranchName(), prid, sha), nil
}

func updateImageTag(path, app, group, tag string) error {
Expand All @@ -94,6 +98,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)
Expand Down
21 changes: 15 additions & 6 deletions pkg/git/azdo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package git
import (
"context"
"fmt"
"log"
"net/url"
"strings"
"time"
Expand Down Expand Up @@ -67,13 +68,14 @@ func NewAzdoGITProvider(ctx context.Context, remoteURL, token string) (*AzdoGITP
}

// CreatePR ...
func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error {
//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)
title := state.Title()
description, err := state.Description()
if err != nil {
return err
return 0, err
}

// Update PR if it already exists
Expand Down Expand Up @@ -104,7 +106,10 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto
GitPullRequestToUpdate: &updatePR,
}
_, err = g.client.UpdatePullRequest(ctx, updateArgs)
return err
if err == nil {
log.Printf("Updated PR #%d merging %s -> %s\n", *pr.PullRequestId, sourceRefName, targetRefName)
}
return *pr.PullRequestId, err
}

// Create new PR
Expand All @@ -124,10 +129,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
Expand All @@ -143,7 +149,10 @@ func (g *AzdoGITProvider) CreatePR(ctx context.Context, branchName string, auto
GitPullRequestToUpdate: &updatePR,
}
_, err = g.client.UpdatePullRequest(ctx, updateArgs)
return err
if err == nil {
log.Printf("Auto-merge activated for PR #%d\n", *pr.PullRequestId)
}
return *pr.PullRequestId, err
}

func (g *AzdoGITProvider) GetStatus(ctx context.Context, sha string, group string, env string) (Status, error) {
Expand Down
31 changes: 23 additions & 8 deletions pkg/git/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io/ioutil"
"log"
"net/http"
"os"
"strings"
Expand Down Expand Up @@ -64,13 +65,14 @@ func NewGitHubGITProvider(ctx context.Context, remoteURL, token string) (*GitHub
}

// CreatePR ...
func (g *GitHubGITProvider) CreatePR(ctx context.Context, branchName string, auto bool, state *PRState) error {
//nolint:gocognit //temporary
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{
Expand All @@ -80,7 +82,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
Expand All @@ -101,18 +103,24 @@ 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))
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) {
Expand All @@ -128,11 +136,17 @@ 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
return pr.GetNumber(), err
}

func (g *GitHubGITProvider) GetStatus(ctx context.Context, sha string, group string, env string) (Status, error) {
Expand All @@ -141,6 +155,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 {
Expand Down
24 changes: 18 additions & 6 deletions pkg/git/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ var _ = Describe("GitHubGITProvider CreatePR", func() {
}
})

var prid int
var err error
var branchName string
var auto bool
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")

Expand All @@ -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))
})
})
})
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/git/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions pkg/git/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package git
import (
"context"
"fmt"
"log"
"path/filepath"
"time"

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -166,10 +169,11 @@ 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
}

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)
}

Expand Down
10 changes: 5 additions & 5 deletions tests/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, fmt.Sprintf("created branch %s with pull request", promoteBranchName))

path = testCloneRepositoryAndValidateTag(t, p.url, p.username, p.password, p.defaultBranch, group, "dev", app, tag)

Expand All @@ -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, 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(
Expand All @@ -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)
Expand All @@ -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, 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(
Expand All @@ -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)
Expand Down