diff --git a/README.md b/README.md index 5803ad7..1b3a770 100644 --- a/README.md +++ b/README.md @@ -206,6 +206,8 @@ Next, to push and raise PRs against changed repos, run: Use `turbolift create-prs --sleep 30s` to, for example, force a 30s pause between creation of each PR. This can be helpful in reducing load on shared infrastructure. +By default Turbolift applies a `turbolift` label to each PR it creates. You can opt out of creating a label with `turbolift create-prs --no-apply-labels`. + > Important: if raising many PRs, you may generate load on shared infrastructure such as CI. It is *highly* recommended that you: > * slow the rate of PR creation by making Turbolift sleep in between PRs > * create PRs in batches, for example by commenting out repositories in `repos.txt` diff --git a/cmd/create_prs/create_prs.go b/cmd/create_prs/create_prs.go index 0eb3244..76a5cfa 100644 --- a/cmd/create_prs/create_prs.go +++ b/cmd/create_prs/create_prs.go @@ -44,6 +44,7 @@ var ( repoFile string prDescriptionFile string sleep time.Duration + noApplyLabels bool ) func NewCreatePRsCmd() *cobra.Command { @@ -55,6 +56,7 @@ func NewCreatePRsCmd() *cobra.Command { cmd.Flags().DurationVar(&sleep, "sleep", 0, "Fixed sleep in between PR creations (to spread load on CI infrastructure)") cmd.Flags().BoolVar(&isDraft, "draft", false, "Creates the Pull Request as Draft PR") + cmd.Flags().BoolVar(&noApplyLabels, "no-apply-labels", false, "Do not apply the default turbolift label to created PRs") cmd.Flags().StringVar(&repoFile, "repos", "repos.txt", "A file containing a list of repositories to clone.") cmd.Flags().StringVar(&prDescriptionFile, "description", "README.md", "A file containing the title and description for the PRs.") @@ -121,6 +123,7 @@ func run(c *cobra.Command, _ []string) { Body: dir.PrBody, UpstreamRepo: repo.FullRepoName, IsDraft: isDraft, + ApplyLabels: !noApplyLabels, } didCreate, err := gh.CreatePullRequest(createPrActivity.Writer(), repoDirPath, pullRequest) diff --git a/cmd/create_prs/create_prs_test.go b/cmd/create_prs/create_prs_test.go index 7fd60a2..bf37984 100644 --- a/cmd/create_prs/create_prs_test.go +++ b/cmd/create_prs/create_prs_test.go @@ -129,8 +129,8 @@ func TestItLogsCreatePrErrorsButContinuesToTryAll(t *testing.T) { assert.Contains(t, out, "2 errored") fakeGitHub.AssertCalledWith(t, [][]string{ - {"create_pull_request", "work/org/repo1", "PR title"}, - {"create_pull_request", "work/org/repo2", "PR title"}, + {"create_pull_request", "work/org/repo1", "PR title", "turbolift"}, + {"create_pull_request", "work/org/repo2", "PR title", "turbolift"}, }) } @@ -150,8 +150,8 @@ func TestItLogsCreatePrSkippedButContinuesToTryAll(t *testing.T) { assert.Contains(t, out, "0 OK, 2 skipped") fakeGitHub.AssertCalledWith(t, [][]string{ - {"create_pull_request", "work/org/repo1", "PR title"}, - {"create_pull_request", "work/org/repo2", "PR title"}, + {"create_pull_request", "work/org/repo1", "PR title", "turbolift"}, + {"create_pull_request", "work/org/repo2", "PR title", "turbolift"}, }) } @@ -169,8 +169,8 @@ func TestItLogsCreatePrsSucceeds(t *testing.T) { assert.Contains(t, out, "2 OK, 0 skipped") fakeGitHub.AssertCalledWith(t, [][]string{ - {"create_pull_request", "work/org/repo1", "PR title"}, - {"create_pull_request", "work/org/repo2", "PR title"}, + {"create_pull_request", "work/org/repo1", "PR title", "turbolift"}, + {"create_pull_request", "work/org/repo2", "PR title", "turbolift"}, }) } @@ -190,8 +190,8 @@ func TestItLogsCreateDraftPr(t *testing.T) { assert.Contains(t, out, "2 OK, 0 skipped") fakeGitHub.AssertCalledWith(t, [][]string{ - {"create_pull_request", "work/org/repo1", "PR title"}, - {"create_pull_request", "work/org/repo2", "PR title"}, + {"create_pull_request", "work/org/repo1", "PR title", "turbolift"}, + {"create_pull_request", "work/org/repo2", "PR title", "turbolift"}, }) } @@ -215,8 +215,27 @@ func TestItCreatesPrsFromAlternativeDescriptionFile(t *testing.T) { assert.Contains(t, out, "2 OK, 0 skipped") fakeGitHub.AssertCalledWith(t, [][]string{ - {"create_pull_request", "work/org/repo1", "custom PR title"}, - {"create_pull_request", "work/org/repo2", "custom PR title"}, + {"create_pull_request", "work/org/repo1", "custom PR title", "turbolift"}, + {"create_pull_request", "work/org/repo2", "custom PR title", "turbolift"}, + }) +} + +func TestItCanSkipApplyingLabels(t *testing.T) { + fakeGitHub := github.NewAlwaysSucceedsFakeGitHub() + gh = fakeGitHub + fakeGit := git.NewAlwaysSucceedsFakeGit() + g = fakeGit + + testsupport.PrepareTempCampaign(true, "org/repo1", "org/repo2") + + out, err := runCommandNoLabels() + assert.NoError(t, err) + assert.Contains(t, out, "turbolift create-prs completed") + assert.Contains(t, out, "2 OK, 0 skipped") + + fakeGitHub.AssertCalledWith(t, [][]string{ + {"create_pull_request", "work/org/repo1", "PR title", ""}, + {"create_pull_request", "work/org/repo2", "PR title", ""}, }) } @@ -228,6 +247,15 @@ func runCommand() (string, error) { return outBuffer.String(), err } +func runCommandNoLabels() (string, error) { + cmd := NewCreatePRsCmd() + cmd.SetArgs([]string{"--no-apply-labels"}) + outBuffer := bytes.NewBufferString("") + cmd.SetOut(outBuffer) + err := cmd.Execute() + return outBuffer.String(), err +} + func runCommandWithAlternativeDescriptionFile(fileName string) (string, error) { cmd := NewCreatePRsCmd() prDescriptionFile = fileName diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go index a0c7925..6c2320d 100644 --- a/internal/github/fake_github.go +++ b/internal/github/fake_github.go @@ -42,7 +42,11 @@ type FakeGitHub struct { } func (f *FakeGitHub) CreatePullRequest(_ io.Writer, workingDir string, metadata PullRequest) (didCreate bool, err error) { - args := []string{"create_pull_request", workingDir, metadata.Title} + labelValue := "" + if metadata.ApplyLabels { + labelValue = TurboliftLabel + } + args := []string{"create_pull_request", workingDir, metadata.Title, labelValue} f.calls = append(f.calls, args) return f.handler(CreatePullRequest, args) } diff --git a/internal/github/github.go b/internal/github/github.go index 898c390..7c9b566 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -27,11 +27,16 @@ import ( var execInstance executor.Executor = executor.NewRealExecutor() +const TurboliftLabel = "turbolift" +const turboliftLabelColor = "0366d6" +const turboliftLabelDescription = "Created using turbolift (github.com/Skyscanner/turbolift)" + type PullRequest struct { Title string Body string UpstreamRepo string IsDraft bool + ApplyLabels bool ReviewDecision string } @@ -49,6 +54,12 @@ type GitHub interface { type RealGitHub struct{} func (r *RealGitHub) CreatePullRequest(output io.Writer, workingDir string, pr PullRequest) (didCreate bool, err error) { + if pr.ApplyLabels { + if err := r.ensureTurboliftLabelExists(output, workingDir, pr.UpstreamRepo); err != nil { + return false, err + } + } + gh_args := []string{ "pr", "create", @@ -60,6 +71,10 @@ func (r *RealGitHub) CreatePullRequest(output io.Writer, workingDir string, pr P pr.UpstreamRepo, } + if pr.ApplyLabels { + gh_args = append(gh_args, "--label", TurboliftLabel) + } + if pr.IsDraft { gh_args = append(gh_args, "--draft") } @@ -74,6 +89,30 @@ func (r *RealGitHub) CreatePullRequest(output io.Writer, workingDir string, pr P return true, nil } +func (r *RealGitHub) ensureTurboliftLabelExists(output io.Writer, workingDir string, repo string) error { + args := []string{ + "label", + "create", + TurboliftLabel, + "--repo", + repo, + "--color", + turboliftLabelColor, + "--description", + turboliftLabelDescription, + } + + stdErr, err := execInstance.ExecuteAndCapture(output, workingDir, "gh", args...) + if err != nil { + if strings.Contains(stdErr, "already exists") || strings.Contains(err.Error(), "already exists") { + return nil + } + return err + } + + return nil +} + func (r *RealGitHub) ForkAndClone(output io.Writer, workingDir string, fullRepoName string) error { return execInstance.Execute(output, workingDir, "gh", "repo", "fork", "--clone=true", fullRepoName) } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 07f44ad..dcd99d3 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -74,7 +74,16 @@ func TestItReturnsNilErrorOnSuccessfulClone(t *testing.T) { } func TestItReturnsErrorOnFailedCreatePr(t *testing.T) { - fakeExecutor := executor.NewAlwaysFailsFakeExecutor() + calls := 0 + fakeExecutor := executor.NewFakeExecutor(func(workingDir string, name string, args ...string) error { + return nil + }, func(workingDir string, name string, args ...string) (string, error) { + calls++ + if calls == 1 { + return "", nil // label create succeeds + } + return "", errors.New("synthetic error") // pr create fails + }) execInstance = fakeExecutor didCreatePr, _, err := runCreatePrAndCaptureOutput() @@ -82,14 +91,20 @@ func TestItReturnsErrorOnFailedCreatePr(t *testing.T) { assert.False(t, didCreatePr) fakeExecutor.AssertCalledWith(t, [][]string{ - {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1"}, + {"work/org/repo1", "gh", "label", "create", "turbolift", "--repo", "org/repo1", "--color", turboliftLabelColor, "--description", turboliftLabelDescription}, + {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--label", "turbolift"}, }) } func TestItReturnsFalseAndNilErrorOnNoOpCreatePr(t *testing.T) { + calls := 0 fakeExecutor := executor.NewFakeExecutor(func(workingDir string, name string, args ...string) error { return nil }, func(workingDir string, name string, args ...string) (string, error) { + calls++ + if calls == 1 { + return "", nil // label create succeeds + } return "... GraphQL error: No commits between A and B ...", errors.New("synthetic error") }) execInstance = fakeExecutor @@ -99,7 +114,8 @@ func TestItReturnsFalseAndNilErrorOnNoOpCreatePr(t *testing.T) { assert.False(t, didCreatePr) fakeExecutor.AssertCalledWith(t, [][]string{ - {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1"}, + {"work/org/repo1", "gh", "label", "create", "turbolift", "--repo", "org/repo1", "--color", turboliftLabelColor, "--description", turboliftLabelDescription}, + {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--label", "turbolift"}, }) } @@ -112,7 +128,8 @@ func TestItSuccessfulCreatesADraftPr(t *testing.T) { assert.True(t, didCreatePr) fakeExecutor.AssertCalledWith(t, [][]string{ - {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--draft"}, + {"work/org/repo1", "gh", "label", "create", "turbolift", "--repo", "org/repo1", "--color", turboliftLabelColor, "--description", turboliftLabelDescription}, + {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--label", "turbolift", "--draft"}, }) } @@ -124,11 +141,48 @@ func TestItReturnsTrueAndNilErrorOnSuccessfulCreatePr(t *testing.T) { assert.NoError(t, err) assert.True(t, didCreatePr) + fakeExecutor.AssertCalledWith(t, [][]string{ + {"work/org/repo1", "gh", "label", "create", "turbolift", "--repo", "org/repo1", "--color", turboliftLabelColor, "--description", turboliftLabelDescription}, + {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--label", "turbolift"}, + }) +} + +func TestItCreatesPrWithoutLabelsWhenNoneProvided(t *testing.T) { + fakeExecutor := executor.NewAlwaysSucceedsFakeExecutor() + execInstance = fakeExecutor + + didCreatePr, _, err := runCreatePrWithoutLabelsAndCaptureOutput() + assert.NoError(t, err) + assert.True(t, didCreatePr) + fakeExecutor.AssertCalledWith(t, [][]string{ {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1"}, }) } +func TestItIgnoresExistingLabelError(t *testing.T) { + calls := 0 + fakeExecutor := executor.NewFakeExecutor(func(workingDir string, name string, args ...string) error { + return nil + }, func(workingDir string, name string, args ...string) (string, error) { + calls++ + if calls == 1 { + return "label already exists", errors.New("synthetic error") + } + return "", nil + }) + execInstance = fakeExecutor + + didCreatePr, _, err := runCreatePrAndCaptureOutput() + assert.NoError(t, err) + assert.True(t, didCreatePr) + + fakeExecutor.AssertCalledWith(t, [][]string{ + {"work/org/repo1", "gh", "label", "create", "turbolift", "--repo", "org/repo1", "--color", turboliftLabelColor, "--description", turboliftLabelDescription}, + {"work/org/repo1", "gh", "pr", "create", "--title", "some title", "--body", "some body", "--repo", "org/repo1", "--label", "turbolift"}, + }) +} + func TestItReturnsErrorOnFailedGetDefaultBranchName(t *testing.T) { fakeExecutor := executor.NewAlwaysFailsFakeExecutor() execInstance = fakeExecutor @@ -197,6 +251,7 @@ func runCreatePrAndCaptureOutput() (bool, string, error) { Title: "some title", Body: "some body", UpstreamRepo: "org/repo1", + ApplyLabels: true, }) return didCreatePr, sb.String(), err @@ -209,6 +264,18 @@ func runCreateDraftPrAndCaptureOutput() (bool, string, error) { Body: "some body", UpstreamRepo: "org/repo1", IsDraft: true, + ApplyLabels: true, + }) + + return didCreatePr, sb.String(), err +} + +func runCreatePrWithoutLabelsAndCaptureOutput() (bool, string, error) { + sb := strings.Builder{} + didCreatePr, err := NewRealGitHub().CreatePullRequest(&sb, "work/org/repo1", PullRequest{ + Title: "some title", + Body: "some body", + UpstreamRepo: "org/repo1", }) return didCreatePr, sb.String(), err