chore: refactor generation result (#2530)

Updates #2378

Author: JoeWang1127

internal/conventionalcommits/conventional_commits.go

@@ -16,6 +16,7 @@ package conventionalcommits
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"regexp"
@@ -41,13 +42,16 @@ var (
 	// literal. The key is followed by ":" and then the value.
 	// e.g., "Reviewed-by: G. Gemini" or "BREAKING CHANGE: an API was changed".
 	footerRegex     = regexp.MustCompile(`^([A-Za-z-]+|` + breakingChangeKey + `):(.*)`)
-	sourceLinkRegex = regexp.MustCompile(`^\[googleapis\/googleapis@(?P<shortSHA>.*)\]\(https:\/\/github\.com\/googleapis\/googleapis\/commit\/(?P<sha>.*)\)$`)
+	sourceLinkRegex = regexp.MustCompile(`^\[googleapis/googleapis@(?P<shortSHA>.*)]\(https://github\.com/googleapis/googleapis/commit/(?P<sha>.*)\)$`)
 	// libraryIDRegex extracts the libraryID from the commit message in a generation PR.
 	// For a generation PR, each commit is expected to have the libraryID in brackets
 	// ('[]').
-	libraryIDRegex = regexp.MustCompile(`\[([^\]]+)\]`)
+	libraryIDRegex = regexp.MustCompile(`\[([^]]+)]`)
 )
 
+// ErrEmptyCommitMessage returns when the commit message is empty.
+var ErrEmptyCommitMessage = errors.New("empty commit message")
+
 // ConventionalCommit represents a parsed conventional commit message.
 // See https://www.conventionalcommits.org/en/v1.0.0/ for details.
 type ConventionalCommit struct {
@@ -129,7 +133,7 @@ func (c *ConventionalCommit) MarshalJSON() ([]byte, error) {
 func ParseCommits(commit *gitrepo.Commit, libraryID string) ([]*ConventionalCommit, error) {
 	message := commit.Message
 	if strings.TrimSpace(message) == "" {
-		return nil, fmt.Errorf("empty commit message")
+		return nil, ErrEmptyCommitMessage
 	}
 	message = extractCommitMessageOverride(message)
 

internal/gitrepo/gitrepo.go

@@ -299,6 +299,7 @@ func (r *LocalRepository) GetCommit(commitHash string) (*Commit, error) {
 
 // GetLatestCommit returns the latest commit of the given path in the repository.
 func (r *LocalRepository) GetLatestCommit(path string) (*Commit, error) {
+	slog.Info("Retrieving the latest commit", "path", path)
 	opt := &git.LogOptions{
 		Order:    git.LogOrderCommitterTime,
 		FileName: &path,

internal/librarian/command.go

@@ -88,6 +88,7 @@ type commitInfo struct {
 	sourceRepo        gitrepo.Repository
 	state             *config.LibrarianState
 	workRoot          string
+	apiOnboarding     bool
 	failedGenerations int
 }
 

internal/librarian/generate.go

@@ -50,6 +50,15 @@ type generateRunner struct {
 	workRoot        string
 }
 
+// generationStatus represents the result of a single library generation.
+type generationStatus struct {
+	// oldCommit is the SHA of the previously generated version of the library.
+	oldCommit string
+	// apiOnboarding is true if an API is being configured and generated for the
+	// first time.
+	apiOnboarding bool
+}
+
 func newGenerateRunner(cfg *config.Config) (*generateRunner, error) {
 	runner, err := newCommandRunner(cfg)
 	if err != nil {
@@ -88,18 +97,20 @@ func (r *generateRunner) run(ctx context.Context) error {
 	// use this map to keep the mapping from library id to commit sha before the
 	// generation since we need these commits to create pull request body.
 	idToCommits := make(map[string]string)
+	apiOnboarding := false
 	var failedLibraries []string
 	failedGenerations := 0
 	if r.api != "" || r.library != "" {
 		libraryID := r.library
 		if libraryID == "" {
 			libraryID = findLibraryIDByAPIPath(r.state, r.api)
 		}
-		oldCommit, err := r.generateSingleLibrary(ctx, libraryID, outputDir)
+		status, err := r.generateSingleLibrary(ctx, libraryID, outputDir)
 		if err != nil {
 			return err
 		}
-		idToCommits[libraryID] = oldCommit
+		idToCommits[libraryID] = status.oldCommit
+		apiOnboarding = status.apiOnboarding
 	} else {
 		succeededGenerations := 0
 		blockedGenerations := 0
@@ -112,15 +123,15 @@ func (r *generateRunner) run(ctx context.Context) error {
 					continue
 				}
 			}
-			oldCommit, err := r.generateSingleLibrary(ctx, library.ID, outputDir)
+			status, err := r.generateSingleLibrary(ctx, library.ID, outputDir)
 			if err != nil {
 				slog.Error("failed to generate library", "id", library.ID, "err", err)
 				failedLibraries = append(failedLibraries, library.ID)
 				failedGenerations++
 			} else {
 				// Only add the mapping if library generation is successful so that
 				// failed library will not appear in generation PR body.
-				idToCommits[library.ID] = oldCommit
+				idToCommits[library.ID] = status.oldCommit
 				succeededGenerations++
 			}
 		}
@@ -154,6 +165,7 @@ func (r *generateRunner) run(ctx context.Context) error {
 		sourceRepo:        r.sourceRepo,
 		state:             r.state,
 		workRoot:          r.workRoot,
+		apiOnboarding:     apiOnboarding,
 		failedGenerations: failedGenerations,
 	}
 
@@ -170,58 +182,65 @@ func (r *generateRunner) run(ctx context.Context) error {
 //
 // 3. Build the library.
 //
-// 4. Update the last generated commit.
-//
-// Returns the last generated commit before the generation and error, if any.
-func (r *generateRunner) generateSingleLibrary(ctx context.Context, libraryID, outputDir string) (string, error) {
+// 4. Update the last generated commit or initial piper id if the library needs configure.
+func (r *generateRunner) generateSingleLibrary(ctx context.Context, libraryID, outputDir string) (*generationStatus, error) {
 	safeLibraryDirectory := getSafeDirectoryName(libraryID)
+	apiOnboarding := false
 	if r.needsConfigure() {
 		slog.Info("library not configured, start initial configuration", "library", r.library)
 		configureOutputDir := filepath.Join(outputDir, safeLibraryDirectory, "configure")
 		if err := os.MkdirAll(configureOutputDir, 0755); err != nil {
-			return "", err
+			return nil, err
 		}
 		configuredLibraryID, err := r.runConfigureCommand(ctx, configureOutputDir)
 		if err != nil {
-			return "", err
+			return nil, err
 		}
+
+		apiOnboarding = true
 		libraryID = configuredLibraryID
 	}
 
 	// At this point, we should have a library in the state.
 	libraryState := findLibraryByID(r.state, libraryID)
 	if libraryState == nil {
-		return "", fmt.Errorf("library %q not configured yet, generation stopped", libraryID)
+		return nil, fmt.Errorf("library %q not configured yet, generation stopped", libraryID)
 	}
 	lastGenCommit := libraryState.LastGeneratedCommit
 
 	if len(libraryState.APIs) == 0 {
 		slog.Info("library has no APIs; skipping generation", "library", libraryID)
-		return "", nil
+		return &generationStatus{
+			oldCommit:     "",
+			apiOnboarding: apiOnboarding,
+		}, nil
 	}
 
 	// For each library, create a separate output directory. This avoids
 	// libraries interfering with each other, and makes it easier to see what
 	// was generated for each library when debugging.
 	libraryOutputDir := filepath.Join(outputDir, safeLibraryDirectory)
 	if err := os.MkdirAll(libraryOutputDir, 0755); err != nil {
-		return "", err
+		return nil, err
 	}
 
 	generatedLibraryID, err := r.runGenerateCommand(ctx, libraryID, libraryOutputDir)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	if err := r.runBuildCommand(ctx, generatedLibraryID); err != nil {
-		return "", err
+		return nil, err
 	}
 
 	if err := r.updateLastGeneratedCommitState(generatedLibraryID); err != nil {
-		return "", err
+		return nil, err
 	}
 
-	return lastGenCommit, nil
+	return &generationStatus{
+		oldCommit:     lastGenCommit,
+		apiOnboarding: apiOnboarding,
+	}, nil
 }
 
 func (r *generateRunner) needsConfigure() bool {
@@ -451,7 +470,7 @@ func setAllAPIStatus(state *config.LibrarianState, status string) {
 
 // getSafeDirectoryName returns a directory name which doesn't contain slashes
 // based on a library ID. This avoids cases where a library ID contains
-// slashes but we want generateSingleLibrary to create a directory which
+// slashes, but we want generateSingleLibrary to create a directory which
 // is not a subdirectory of some other directory. For example, if there
 // are library IDs of "pubsub" and "pubsub/v2" we don't want to create
 // "output/pubsub/v2" and then "output/pubsub" later. This function does

internal/librarian/generate_test.go

@@ -638,7 +638,7 @@ func TestGenerateScenarios(t *testing.T) {
 		wantConfigureCalls int
 	}{
 		{
-			name:    "generate single library including initial configuration",
+			name:    "generate_single_library_including_initial_configuration",
 			api:     "some/api",
 			library: "some-library",
 			state: &config.LibrarianState{