refactor: commands provide PR body builder (#2574)

`commitAndPush` should not be responsible for building the PR body.
Because building the PR body could be a relatively expensive operation,
we lazily build it. The caller of `commitAndPush` provides a callback
func that returns the PR body that is used for creating the GitHub pull
request.

An added benefit is that the PR body building logic can live with the
code that is opening the PR.

---------

Signed-off-by: Jeff Ching <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: chingor13

internal/librarian/command.go

@@ -44,6 +44,8 @@ For each failed library, open a ticket in that library’s repository and then y
 `
 )
 
+var errBuilderNotProvided = fmt.Errorf("no prBodyBuilder provided")
+
 type pullRequestType int
 
 const (
@@ -98,9 +100,7 @@ type commitInfo struct {
 	branch            string
 	commit            bool
 	commitMessage     string
-	failedLibraries   []string
 	ghClient          GitHubClient
-	idToCommits       map[string]string
 	prType            pullRequestType
 	pullRequestLabels []string
 	push              bool
@@ -112,7 +112,8 @@ type commitInfo struct {
 	// api is the api path of a library, only set this value during api onboarding.
 	api string
 	// library is the ID of a library, only set this value during api onboarding.
-	library string
+	library       string
+	prBodyBuilder func() (string, error)
 }
 
 type commandRunner struct {
@@ -359,8 +360,7 @@ func getDirectoryFilenames(dir string) ([]string, error) {
 func commitAndPush(ctx context.Context, info *commitInfo) error {
 	if !info.push && !info.commit {
 		slog.Info("Push flag and Commit flag are not specified, skipping committing")
-		writePRBody(info)
-		return nil
+		return writePRBody(info)
 	}
 
 	repo := info.languageRepo
@@ -389,8 +389,7 @@ func commitAndPush(ctx context.Context, info *commitInfo) error {
 
 	if !info.push {
 		slog.Info("Push flag is not specified, skipping pull request creation")
-		writePRBody(info)
-		return nil
+		return writePRBody(info)
 	}
 
 	if err := repo.Push(branch); err != nil {
@@ -403,7 +402,7 @@ func commitAndPush(ctx context.Context, info *commitInfo) error {
 	}
 
 	title := fmt.Sprintf("chore: librarian %s pull request: %s", info.prType, datetimeNow)
-	prBody, err := createPRBody(info, gitHubRepo)
+	prBody, err := info.prBodyBuilder()
 	if err != nil {
 		return fmt.Errorf("failed to create pull request body: %w", err)
 	}
@@ -423,19 +422,17 @@ func commitAndPush(ctx context.Context, info *commitInfo) error {
 }
 
 // writePRBody attempts to log the body of a PR that would have been created if the
-// -push flag had been specified. This logs any errors (e.g. if the GitHub repo can't be determined)
-// but deliberately does not return them, as a failure here should not interfere with the flow.
-func writePRBody(info *commitInfo) {
-	gitHubRepo, err := GetGitHubRepositoryFromGitRepo(info.languageRepo)
-	if err != nil {
-		slog.Warn("Unable to create PR body; could not determine GitHub repo", "error", err)
-		return
+// -push flag had been specified. This logs any errors and returns them to the
+// caller.
+func writePRBody(info *commitInfo) error {
+	if info.prBodyBuilder == nil {
+		return errBuilderNotProvided
 	}
 
-	prBody, err := createPRBody(info, gitHubRepo)
+	prBody, err := info.prBodyBuilder()
 	if err != nil {
 		slog.Warn("Unable to create PR body", "error", err)
-		return
+		return err
 	}
 	// Note: we can't accurately predict whether a PR would have been created,
 	// as we're not checking whether the repo is clean or not. The intention is to be
@@ -444,11 +441,12 @@ func writePRBody(info *commitInfo) {
 	// Ensure that "cat [path-to-pr-body.txt]" gives useful output.
 	prBody = prBody + "\n"
 	err = os.WriteFile(fullPath, []byte(prBody), 0644)
-	if err == nil {
-		slog.Info("Wrote body of pull request that might have been created", "file", fullPath)
-	} else {
+	if err != nil {
 		slog.Warn("Unable to save PR body", "error", err)
+		return err
 	}
+	slog.Info("Wrote body of pull request that might have been created", "file", fullPath)
+	return nil
 }
 
 // addLabelsToPullRequest adds a list of labels to a single pull request (specified by the id number).
@@ -468,32 +466,6 @@ func addLabelsToPullRequest(ctx context.Context, ghClient GitHubClient, pullRequ
 	return nil
 }
 
-func createPRBody(info *commitInfo, gitHubRepo *github.Repository) (string, error) {
-	switch info.prType {
-	case pullRequestOnboard:
-		req := &onboardPRRequest{
-			sourceRepo: info.sourceRepo,
-			state:      info.state,
-			api:        info.api,
-			library:    info.library,
-		}
-		return formatOnboardPRBody(req)
-	case pullRequestGenerate:
-		req := &generationPRRequest{
-			sourceRepo:      info.sourceRepo,
-			languageRepo:    info.languageRepo,
-			state:           info.state,
-			idToCommits:     info.idToCommits,
-			failedLibraries: info.failedLibraries,
-		}
-		return formatGenerationPRBody(req)
-	case pullRequestRelease:
-		return formatReleaseNotes(info.state, gitHubRepo)
-	default:
-		return "", fmt.Errorf("unrecognized pull request type: %s", info.prType)
-	}
-}
-
 // copyGlobalAllowlist copies files in the global file allowlist from src to dst.
 func copyGlobalAllowlist(cfg *config.LibrarianConfig, dst, src string, copyReadOnly bool) error {
 	if cfg == nil {

internal/librarian/command_test.go

@@ -17,6 +17,7 @@ package librarian
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -1302,6 +1303,7 @@ func TestCommitAndPush(t *testing.T) {
 		expectedErrMsg    string
 		check             func(t *testing.T, repo gitrepo.Repository)
 		wantPRBodyFile    bool
+		prBodyBuilder     func() (string, error)
 	}{
 		{
 			name: "Push flag and Commit flag are not specified",
@@ -1520,6 +1522,7 @@ func TestCommitAndPush(t *testing.T) {
 			push:           true,
 			wantErr:        true,
 			expectedErrMsg: "failed to create pull request body",
+			prBodyBuilder:  func() (string, error) { return "", fmt.Errorf("failed to create pull request body") },
 		},
 		{
 			name: "Create pull request error",
@@ -1593,6 +1596,11 @@ func TestCommitAndPush(t *testing.T) {
 			repo := test.setupMockRepo(t)
 			client := test.setupMockClient(t)
 
+			// set default PR body builder
+			if test.prBodyBuilder == nil {
+				test.prBodyBuilder = func() (string, error) { return "some pr body", nil }
+			}
+
 			commitInfo := &commitInfo{
 				commit:            test.commit,
 				commitMessage:     "",
@@ -1603,6 +1611,7 @@ func TestCommitAndPush(t *testing.T) {
 				state:             test.state,
 				failedGenerations: test.failedGenerations,
 				workRoot:          t.TempDir(),
+				prBodyBuilder:     test.prBodyBuilder,
 			}
 
 			err := commitAndPush(context.Background(), commitInfo)
@@ -1637,6 +1646,7 @@ func TestWritePRBody(t *testing.T) {
 	for _, test := range []struct {
 		name     string
 		info     *commitInfo
+		wantErr  bool
 		wantFile bool
 	}{
 		{
@@ -1651,14 +1661,15 @@ func TestWritePRBody(t *testing.T) {
 						},
 					},
 				},
-				prType:   pullRequestRelease,
-				state:    &config.LibrarianState{},
-				workRoot: t.TempDir(),
+				prType:        pullRequestRelease,
+				state:         &config.LibrarianState{},
+				workRoot:      t.TempDir(),
+				prBodyBuilder: func() (string, error) { return "some pr body", nil },
 			},
 			wantFile: true,
 		},
 		{
-			name: "invalid repo",
+			name: "unable to build PR body",
 			info: &commitInfo{
 				languageRepo: &MockRepository{
 					Dir: t.TempDir(),
@@ -1669,12 +1680,14 @@ func TestWritePRBody(t *testing.T) {
 						},
 					},
 				},
-				prType:   pullRequestRelease,
-				workRoot: t.TempDir(),
+				prType:        pullRequestRelease,
+				workRoot:      t.TempDir(),
+				prBodyBuilder: func() (string, error) { return "", fmt.Errorf("some error") },
 			},
+			wantErr: true,
 		},
 		{
-			name: "invalid-pr-type",
+			name: "unable to save file",
 			info: &commitInfo{
 				languageRepo: &MockRepository{
 					Dir: t.TempDir(),
@@ -1685,12 +1698,15 @@ func TestWritePRBody(t *testing.T) {
 						},
 					},
 				},
-				prType:   4,
-				workRoot: t.TempDir(),
+				prType:        pullRequestRelease,
+				state:         &config.LibrarianState{},
+				workRoot:      filepath.Join(t.TempDir(), "missing-directory"),
+				prBodyBuilder: func() (string, error) { return "some pr body", nil },
 			},
+			wantErr: true,
 		},
 		{
-			name: "unable to save file",
+			name: "no body builder",
 			info: &commitInfo{
 				languageRepo: &MockRepository{
 					Dir: t.TempDir(),
@@ -1705,10 +1721,22 @@ func TestWritePRBody(t *testing.T) {
 				state:    &config.LibrarianState{},
 				workRoot: filepath.Join(t.TempDir(), "missing-directory"),
 			},
+			wantErr: true,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			writePRBody(test.info)
+			err := writePRBody(test.info)
+
+			if test.wantErr {
+				if err == nil {
+					t.Fatalf("writePRBody() expected error, but no error returned")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error %v", err)
+			}
+
 			gotFile := gotPRBodyFile(t, test.info.workRoot)
 			if test.wantFile != gotFile {
 				t.Errorf("writePRBody() wantFile = %t, gotFile = %t", test.wantFile, gotFile)

internal/librarian/generate_command.go

@@ -149,13 +149,38 @@ func (r *generateRunner) run(ctx context.Context) error {
 		return err
 	}
 
+	var prBodyBuilder func() (string, error)
+	switch prType {
+	case pullRequestGenerate:
+		prBodyBuilder = func() (string, error) {
+			req := &generationPRRequest{
+				sourceRepo:      r.sourceRepo,
+				languageRepo:    r.repo,
+				state:           r.state,
+				idToCommits:     idToCommits,
+				failedLibraries: failedLibraries,
+			}
+			return formatGenerationPRBody(req)
+		}
+	case pullRequestOnboard:
+		prBodyBuilder = func() (string, error) {
+			req := &onboardPRRequest{
+				sourceRepo: r.sourceRepo,
+				state:      r.state,
+				api:        r.api,
+				library:    r.library,
+			}
+			return formatOnboardPRBody(req)
+		}
+	default:
+		return fmt.Errorf("unexpected prType %s", prType)
+	}
+
 	commitInfo := &commitInfo{
 		branch:            r.branch,
 		commit:            r.commit,
 		commitMessage:     "feat: generate libraries",
-		failedLibraries:   failedLibraries,
 		ghClient:          r.ghClient,
-		idToCommits:       idToCommits,
 		prType:            prType,
 		push:              r.push,
 		languageRepo:      r.repo,
@@ -165,6 +190,7 @@ func (r *generateRunner) run(ctx context.Context) error {
 		api:               r.api,
 		library:           r.library,
 		failedGenerations: failedGenerations,
+		prBodyBuilder:     prBodyBuilder,
 	}
 
 	return commitAndPush(ctx, commitInfo)