Update PR descriptions (#117)

* add update PR description * add github tests * tests for updating via default readme * custom description file, instructions * Update README.md Co-authored-by: Richard North <[email protected]> * Update README.md Co-authored-by: Richard North <[email protected]> * test for default description file --------- Co-authored-by: Danny Ranson <[email protected]> Co-authored-by: Richard North <[email protected]>
Skyscanner · Apr 9, 2024 · 9c33426 · 9c33426
1 parent 36daf83
commit 9c33426
Show file tree

Hide file tree

Showing 8 changed files with 236 additions and 13 deletions.
diff --git a/README.md b/README.md
@@ -217,13 +217,27 @@ redacted/redacted                                         OPEN    REVIEW_REQUIRE
 ...
 ```
 
-#### Closing all PRs
+#### Updating PRs
 
-To close all PRs currently opened under the campaign, there is a `--close` flag:
+Use the `update-prs` command to update PRs after creating them. Current options for updating PRs are:
+
+##### Update PR titles and descriptions with `--amend-description`
+
+```turbolift update-prs --amend-description [--yes]```
+
+By default, turbolift will read a revised PR Title and Description from `README.md`. The title is taken from the first heading line, and the description is the remainder of the file contents.
+
+As with creating PRs, if you need Turbolift to read these values from an alternative file, use the flag `--description PATH_TO_FILE`.
+
+```turblift update-prs --amend-description --description prDescriptionFile1.md```
+
+##### Close PRs with the `--close` flag
 
 ```turbolift update-prs --close [--yes]```
 
-If the flag `--yes` is not present, a confirmation prompt will be presented to the user.
+If the flag `--yes` is not passed with an `update-prs` command, a confirmation prompt will be presented to the user.
+
+As always, use the `--repos` flag to specify an alternative repo file to repos.txt.
 
 ## Status: Preview
 

diff --git a/cmd/updateprs/updateprs.go b/cmd/updateprs/updateprs.go
@@ -35,9 +35,11 @@ var (
 )
 
 var (
-	closeFlag bool
-	yesFlag   bool
-	repoFile  string
+	closeFlag             bool
+	updateDescriptionFlag bool
+	yesFlag               bool
+	repoFile              string
+	prDescriptionFile     string
 )
 
 func NewUpdatePRsCmd() *cobra.Command {
@@ -48,8 +50,10 @@ func NewUpdatePRsCmd() *cobra.Command {
 	}
 
 	cmd.Flags().BoolVar(&closeFlag, "close", false, "Close all generated PRs")
+	cmd.Flags().BoolVar(&updateDescriptionFlag, "amend-description", false, "Update PR titles and descriptions")
 	cmd.Flags().BoolVar(&yesFlag, "yes", false, "Skips the confirmation prompt")
 	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.")
 
 	return cmd
 }
@@ -67,9 +71,8 @@ func onlyOne(args ...bool) bool {
 	return b[true] == 1
 }
 
-func validateFlags(closeFlag bool) error {
-	// only option at the moment is `close`
-	if !onlyOne(closeFlag) {
+func validateFlags(closeFlag bool, updateDescriptionFlag bool) error {
+	if !onlyOne(closeFlag, updateDescriptionFlag) {
 		return errors.New("update-prs needs one and only one action flag")
 	}
 	return nil
@@ -78,13 +81,15 @@ func validateFlags(closeFlag bool) error {
 // we keep the args as one of the subfunctions might need it one day.
 func run(c *cobra.Command, args []string) {
 	logger := logging.NewLogger(c)
-	if err := validateFlags(closeFlag); err != nil {
+	if err := validateFlags(closeFlag, updateDescriptionFlag); err != nil {
 		logger.Errorf("Error while parsing the flags: %v", err)
 		return
 	}
 
 	if closeFlag {
 		runClose(c, args)
+	} else if updateDescriptionFlag {
+		runUpdatePrDescription(c, args)
 	}
 }
 
@@ -104,7 +109,7 @@ func runClose(c *cobra.Command, _ []string) {
 	// Prompting for confirmation
 	if !yesFlag {
 		// TODO: add the number of PRs that it will actually close
-		if !p.AskConfirm(fmt.Sprintf("Close all PRs from the %s campaign?", dir.Name)) {
+		if !p.AskConfirm(fmt.Sprintf("Close %s campaign PRs for all repos in %s?", dir.Name, repoFile)) {
 			return
 		}
 	}
@@ -144,3 +149,60 @@ func runClose(c *cobra.Command, _ []string) {
 		logger.Warnf("turbolift update-prs completed with %s %s(%s, %s, %s)\n", colors.Red("errors"), colors.Normal(), colors.Green(doneCount, " OK"), colors.Yellow(skippedCount, " skipped"), colors.Red(errorCount, " errored"))
 	}
 }
+
+func runUpdatePrDescription(c *cobra.Command, _ []string) {
+	logger := logging.NewLogger(c)
+
+	readCampaignActivity := logger.StartActivity("Reading campaign data (%s)", repoFile)
+	options := campaign.NewCampaignOptions()
+	options.RepoFilename = repoFile
+	options.PrDescriptionFilename = prDescriptionFile
+	dir, err := campaign.OpenCampaign(options)
+	if err != nil {
+		readCampaignActivity.EndWithFailure(err)
+		return
+	}
+	readCampaignActivity.EndWithSuccess()
+
+	// Prompting for confirmation
+	if !yesFlag {
+		if !p.AskConfirm(fmt.Sprintf("Update %s campaign PR titles and descriptions for all repos listed in %s?", dir.Name, repoFile)) {
+			return
+		}
+	}
+
+	doneCount := 0
+	skippedCount := 0
+	errorCount := 0
+
+	for _, repo := range dir.Repos {
+		updatePrActivity := logger.StartActivity("Updating PR description in %s", repo.FullRepoName)
+
+		// skip if the working copy does not exist
+		if _, err = os.Stat(repo.FullRepoPath()); os.IsNotExist(err) {
+			updatePrActivity.EndWithWarningf("Directory %s does not exist - has it been cloned?", repo.FullRepoPath())
+			skippedCount++
+			continue
+		}
+
+		err = gh.UpdatePRDescription(updatePrActivity.Writer(), repo.FullRepoPath(), dir.PrTitle, dir.PrBody)
+		if err != nil {
+			if _, ok := err.(*github.NoPRFoundError); ok {
+				updatePrActivity.EndWithWarning(err)
+				skippedCount++
+			} else {
+				updatePrActivity.EndWithFailure(err)
+				errorCount++
+			}
+		} else {
+			updatePrActivity.EndWithSuccess()
+			doneCount++
+		}
+	}
+
+	if errorCount == 0 {
+		logger.Successf("turbolift update-prs completed %s(%s, %s)\n", colors.Normal(), colors.Green(doneCount, " OK"), colors.Yellow(skippedCount, " skipped"))
+	} else {
+		logger.Warnf("turbolift update-prs completed with %s %s(%s, %s, %s)\n", colors.Red("errors"), colors.Normal(), colors.Green(doneCount, " OK"), colors.Yellow(skippedCount, " skipped"), colors.Red(errorCount, " errored"))
+	}
+}
diff --git a/cmd/updateprs/updateprs_test.go b/cmd/updateprs/updateprs_test.go
@@ -88,6 +88,83 @@ func TestItDoesNotClosePRsIfNotConfirmed(t *testing.T) {
 	fakeGitHub.AssertCalledWith(t, [][]string{})
 }
 
+func TestItLogsUpdateDescriptionErrorsButContinuesToTryAll(t *testing.T) {
+	fakeGitHub := github.NewAlwaysFailsFakeGitHub()
+	gh = fakeGitHub
+
+	testsupport.PrepareTempCampaign(true, "org/repo1", "org/repo2")
+
+	out, err := runUpdateDescriptionCommandAuto("README.md")
+	assert.NoError(t, err)
+	assert.Contains(t, out, "Updating PR description in org/repo1")
+	assert.Contains(t, out, "Updating PR description in org/repo2")
+	assert.Contains(t, out, "turbolift update-prs completed with errors")
+	assert.Contains(t, out, "2 errored")
+
+	fakeGitHub.AssertCalledWith(t, [][]string{
+		{"work/org/repo1", "PR title", "PR body"},
+		{"work/org/repo2", "PR title", "PR body"},
+	})
+}
+
+func TestItUpdatesDescriptionsSuccessfully(t *testing.T) {
+	fakeGitHub := github.NewAlwaysSucceedsFakeGitHub()
+	gh = fakeGitHub
+
+	testsupport.PrepareTempCampaign(true, "org/repo1", "org/repo2")
+	testsupport.CreateOrUpdatePrDescriptionFile("README.md", "Updated PR title", "Updated PR body")
+
+	out, err := runUpdateDescriptionCommandAuto("README.md")
+	assert.NoError(t, err)
+	assert.Contains(t, out, "Updating PR description in org/repo1")
+	assert.Contains(t, out, "Updating PR description in org/repo2")
+	assert.Contains(t, out, "turbolift update-prs completed")
+	assert.Contains(t, out, "2 OK, 0 skipped")
+
+	fakeGitHub.AssertCalledWith(t, [][]string{
+		{"work/org/repo1", "Updated PR title", "Updated PR body"},
+		{"work/org/repo2", "Updated PR title", "Updated PR body"},
+	})
+}
+
+func TestItUpdatesDescriptionsFromAlternativeFile(t *testing.T) {
+	fakeGitHub := github.NewAlwaysSucceedsFakeGitHub()
+	gh = fakeGitHub
+
+	testsupport.PrepareTempCampaign(true, "org/repo1", "org/repo2")
+	testsupport.CreateOrUpdatePrDescriptionFile("custom.md", "custom PR title", "custom PR body")
+
+	out, err := runUpdateDescriptionCommandAuto("custom.md")
+	assert.NoError(t, err)
+	assert.Contains(t, out, "Updating PR description in org/repo1")
+	assert.Contains(t, out, "Updating PR description in org/repo2")
+	assert.Contains(t, out, "turbolift update-prs completed")
+	assert.Contains(t, out, "2 OK, 0 skipped")
+
+	fakeGitHub.AssertCalledWith(t, [][]string{
+		{"work/org/repo1", "custom PR title", "custom PR body"},
+		{"work/org/repo2", "custom PR title", "custom PR body"},
+	})
+}
+
+func TestItDoesNotUpdateDescriptionsIfNotConfirmed(t *testing.T) {
+	fakeGitHub := github.NewAlwaysSucceedsFakeGitHub()
+	gh = fakeGitHub
+	fakePrompt := prompt.NewFakePromptNo()
+	p = fakePrompt
+
+	testsupport.PrepareTempCampaign(true, "org/repo1", "org/repo2")
+
+	out, err := runUpdateDescriptionCommandConfirm()
+	assert.NoError(t, err)
+	assert.NotContains(t, out, "Updating PR description in org/repo1")
+	assert.NotContains(t, out, "Updating PR description in org/repo2")
+	assert.NotContains(t, out, "turbolift update-prs completed")
+	assert.NotContains(t, out, "2 OK")
+
+	fakeGitHub.AssertCalledWith(t, [][]string{})
+}
+
 func runCloseCommandAuto() (string, error) {
 	cmd := NewUpdatePRsCmd()
 	closeFlag = true
@@ -113,3 +190,30 @@ func runCloseCommandConfirm() (string, error) {
 	}
 	return outBuffer.String(), nil
 }
+
+func runUpdateDescriptionCommandAuto(descriptionFile string) (string, error) {
+	cmd := NewUpdatePRsCmd()
+	updateDescriptionFlag = true
+	yesFlag = true
+	prDescriptionFile = descriptionFile
+	outBuffer := bytes.NewBufferString("")
+	cmd.SetOut(outBuffer)
+	err := cmd.Execute()
+	if err != nil {
+		return outBuffer.String(), err
+	}
+	return outBuffer.String(), nil
+}
+
+func runUpdateDescriptionCommandConfirm() (string, error) {
+	cmd := NewUpdatePRsCmd()
+	updateDescriptionFlag = true
+	yesFlag = false
+	outBuffer := bytes.NewBufferString("")
+	cmd.SetOut(outBuffer)
+	err := cmd.Execute()
+	if err != nil {
+		return outBuffer.String(), err
+	}
+	return outBuffer.String(), nil
+}
diff --git a/internal/campaign/campaign_test.go b/internal/campaign/campaign_test.go
@@ -227,7 +227,7 @@ func TestItShouldErrorWhenRepoFileIsEmpty(t *testing.T) {
 func TestItShouldAcceptADifferentPrDescriptionFile(t *testing.T) {
 	testsupport.PrepareTempCampaign(false)
 
-	testsupport.CreateAnotherPrDescriptionFile("newprdescription.txt", "new PR title", "new PR body")
+	testsupport.CreateOrUpdatePrDescriptionFile("newprdescription.txt", "new PR title", "new PR body")
 	options := NewCampaignOptions()
 	options.PrDescriptionFilename = "newprdescription.txt"
 	campaign, err := OpenCampaign(options)

diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go
@@ -72,6 +72,13 @@ func (f *FakeGitHub) GetDefaultBranchName(_ io.Writer, workingDir string, fullRe
 	return "main", err
 }
 
+func (f *FakeGitHub) UpdatePRDescription(_ io.Writer, workingDir string, title string, body string) error {
+	args := []string{workingDir, title, body}
+	f.calls = append(f.calls, args)
+	_, err := f.handler(UpdatePRDescription, args)
+	return err
+}
+
 func (f *FakeGitHub) AssertCalledWith(t *testing.T, expected [][]string) {
 	assert.Equal(t, expected, f.calls)
 }
@@ -136,4 +143,5 @@ const (
 	CreatePullRequest
 	ClosePullRequest
 	GetDefaultBranchName
+	UpdatePRDescription
 )
diff --git a/internal/github/github.go b/internal/github/github.go
@@ -39,6 +39,7 @@ type GitHub interface {
 	Clone(output io.Writer, workingDir string, fullRepoName string) error
 	CreatePullRequest(output io.Writer, workingDir string, metadata PullRequest) (didCreate bool, err error)
 	ClosePullRequest(output io.Writer, workingDir string, branchName string) error
+	UpdatePRDescription(output io.Writer, workingDir string, title string, body string) error
 	GetPR(output io.Writer, workingDir string, branchName string) (*PrStatus, error)
 	GetDefaultBranchName(output io.Writer, workingDir string, fullRepoName string) (string, error)
 }
@@ -88,6 +89,10 @@ func (r *RealGitHub) ClosePullRequest(output io.Writer, workingDir string, branc
 	return execInstance.Execute(output, workingDir, "gh", "pr", "close", fmt.Sprint(pr.Number))
 }
 
+func (r *RealGitHub) UpdatePRDescription(output io.Writer, workingDir string, title string, body string) error {
+	return execInstance.Execute(output, workingDir, "gh", "pr", "edit", "--title", title, "--body", body)
+}
+
 func (r *RealGitHub) GetDefaultBranchName(output io.Writer, workingDir string, fullRepoName string) (string, error) {
 	defaultBranch, err := execInstance.ExecuteAndCapture(output, workingDir, "gh", "repo", "view", fullRepoName, "--json", "defaultBranchRef", "--jq", ".defaultBranchRef.name")
 	return strings.Trim(defaultBranch, "\n"), err

diff --git a/internal/github/github_test.go b/internal/github/github_test.go
@@ -151,6 +151,30 @@ func TestItReturnsNilErrorOnSuccessfulGetDefaultBranchName(t *testing.T) {
 	})
 }
 
+func TestItReturnsErrorOnFailedUpdatePrDescription(t *testing.T) {
+	fakeExecutor := executor.NewAlwaysFailsFakeExecutor()
+	execInstance = fakeExecutor
+
+	_, err := runUpdatePrDescriptionAndCaptureOutput()
+	assert.Error(t, err)
+
+	fakeExecutor.AssertCalledWith(t, [][]string{
+		{"work/org/repo1", "gh", "pr", "edit", "--title", "new title", "--body", "new body"},
+	})
+}
+
+func TestItReturnsNilErrorOnSuccessfulUpdatePrDescription(t *testing.T) {
+	fakeExecutor := executor.NewAlwaysSucceedsFakeExecutor()
+	execInstance = fakeExecutor
+
+	_, err := runUpdatePrDescriptionAndCaptureOutput()
+	assert.NoError(t, err)
+
+	fakeExecutor.AssertCalledWith(t, [][]string{
+		{"work/org/repo1", "gh", "pr", "edit", "--title", "new title", "--body", "new body"},
+	})
+}
+
 func runForkAndCloneAndCaptureOutput() (string, error) {
 	sb := strings.Builder{}
 	err := NewRealGitHub().ForkAndClone(&sb, "work/org", "org/repo1")
@@ -193,3 +217,9 @@ func runGetDefaultBranchNameAndCaptureOutput() (string, string, error) {
 	defaultBranchName, err := NewRealGitHub().GetDefaultBranchName(&sb, "work/org1/repo1", "org1/repo1")
 	return defaultBranchName, sb.String(), err
 }
+
+func runUpdatePrDescriptionAndCaptureOutput() (string, error) {
+	sb := strings.Builder{}
+	err := NewRealGitHub().UpdatePRDescription(&sb, "work/org/repo1", "new title", "new body")
+	return sb.String(), err
+}
diff --git a/internal/testsupport/testsupport.go b/internal/testsupport/testsupport.go
@@ -74,7 +74,7 @@ func CreateAnotherRepoFile(filename string, repos ...string) {
 	}
 }
 
-func CreateAnotherPrDescriptionFile(filename string, prTitle string, prBody string) {
+func CreateOrUpdatePrDescriptionFile(filename string, prTitle string, prBody string) {
 	prDescription := fmt.Sprintf("# %s\n%s", prTitle, prBody)
 	err := os.WriteFile(filename, []byte(prDescription), os.ModePerm|0o644)
 	if err != nil {