From a5b32d06b071bc69f93de3df0331edf487e85802 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Mon, 20 Oct 2025 09:47:49 +0000 Subject: [PATCH 1/5] feat(internal/librarian): only generate libraries with changed APIs The new -generate-unchanged flag can be used to generate all (unblocked) libraries. Libraries which are blocked by configuration are still not generated when -generate-unchanged is specified. Specifying -library ignores this check (and blocking) and will always generate the specified library. Fixes #818 --- cmd/librarian/doc.go | 3 + internal/config/config.go | 8 +- internal/gitrepo/gitrepo.go | 17 ++ internal/gitrepo/gitrepo_test.go | 87 ++++++-- internal/librarian/flags.go | 6 + internal/librarian/generate_command.go | 151 +++++++++---- internal/librarian/generate_command_test.go | 228 +++++++++++++++++++- internal/librarian/generate_pull_request.go | 4 +- internal/librarian/librarian.go | 1 + internal/librarian/mocks_test.go | 18 ++ 10 files changed, 456 insertions(+), 67 deletions(-) diff --git a/cmd/librarian/doc.go b/cmd/librarian/doc.go index d29b3610eb..54d456a253 100644 --- a/cmd/librarian/doc.go +++ b/cmd/librarian/doc.go @@ -104,6 +104,9 @@ Flags: -build If true, Librarian will build each generated library by invoking the language-specific container. + -generate-unchanged + If true, librarian generates libraries even if none of their associated APIs + have changed. This does not override generation being blocked by configuration. -host-mount string For use when librarian is running in a container. A mapping of a directory from the host to the container, in the format diff --git a/internal/config/config.go b/internal/config/config.go index 73571072a2..2597b0a95a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -123,12 +123,18 @@ type Config struct { // expected. CommandName string - // Commit determines whether to creat a commit for the release but not create + // Commit determines whether to create a commit for the release but not create // a pull request. // // This flag is ignored if Push is set to true. Commit bool + // GenerateUnchanged determines whether to generate libraries where none of + // the associated APIs have changed since the commit at which they were last + // generated. Note that this does not override any configuration indicating + // that the library should not be automatically generated. + GenerateUnchanged bool + // GitHubAPIEndpoint is the GitHub API endpoint to use for all GitHub API // operations. // diff --git a/internal/gitrepo/gitrepo.go b/internal/gitrepo/gitrepo.go index 1d66ffeece..34bde0aae9 100644 --- a/internal/gitrepo/gitrepo.go +++ b/internal/gitrepo/gitrepo.go @@ -55,6 +55,7 @@ type Repository interface { CleanUntracked(paths []string) error pushRefSpec(refSpec string) error Checkout(commitHash string) error + GetHashForPathOrEmpty(commitHash, path string) (string, error) } const RootPath = "." @@ -663,3 +664,19 @@ func (r *LocalRepository) Checkout(commitSha string) error { Hash: plumbing.NewHash(commitSha), }) } + +// GetHashForPathOrEmpty returns a tree hash for the specified path, +// at the given commit in this repository. If the path does not exist +// at the commit, an empty string is returned rather than an error, +// as the purpose of this function is to allow callers to determine changes +// in the tree. (A path going from missing to anything else, or vice versa, +// indicates a change. A path being missing at two different commits is not a change.) +func (r *LocalRepository) GetHashForPathOrEmpty(commitHash, path string) (string, error) { + // This public function just delegates to the internal function that uses a Commit + // object instead of the hash. + commit, err := r.repo.CommitObject(plumbing.NewHash(commitHash)) + if err != nil { + return "", err + } + return getHashForPathOrEmpty(commit, path) +} diff --git a/internal/gitrepo/gitrepo_test.go b/internal/gitrepo/gitrepo_test.go index 5dcedd416b..6d9b008776 100644 --- a/internal/gitrepo/gitrepo_test.go +++ b/internal/gitrepo/gitrepo_test.go @@ -904,27 +904,34 @@ func TestGetDir(t *testing.T) { } } +// TestGetHashForPathOrEmpty tests the internal getHashForPathOrEmpty, but +// via the public GetHashForPathOrEmpty function which accepts a commit hash +// instead of a Commit object, to avoid duplicate testing. func TestGetHashForPathOrEmpty(t *testing.T) { t.Parallel() - setupInitialRepo := func(t *testing.T) (*git.Repository, *object.Commit) { + setupInitialRepo := func(t *testing.T) (LocalRepository, *object.Commit) { t.Helper() - repo, _ := initTestRepo(t) + repo, dir := initTestRepo(t) commit := createAndCommit(t, repo, "initial.txt", []byte("initial content"), "initial commit") - return repo, commit + localRepository := LocalRepository{ + Dir: dir, + repo: repo, + } + return localRepository, commit } for _, test := range []struct { name string - setup func(t *testing.T) (commit *object.Commit, path string) + setup func(t *testing.T) (repo LocalRepository, commit *object.Commit, path string) wantHash func(commit *object.Commit, path string) string wantErr bool }{ { name: "existing file", - setup: func(t *testing.T) (*object.Commit, string) { - _, commit := setupInitialRepo(t) - return commit, "initial.txt" + setup: func(t *testing.T) (LocalRepository, *object.Commit, string) { + localRepository, commit := setupInitialRepo(t) + return localRepository, commit, "initial.txt" }, wantHash: func(commit *object.Commit, path string) string { tree, err := commit.Tree() @@ -940,8 +947,9 @@ func TestGetHashForPathOrEmpty(t *testing.T) { }, { name: "existing directory", - setup: func(t *testing.T) (*object.Commit, string) { - repo, _ := setupInitialRepo(t) + setup: func(t *testing.T) (LocalRepository, *object.Commit, string) { + localRepository, _ := setupInitialRepo(t) + repo := localRepository.repo // Create a directory and a file inside it to ensure the directory gets a hash _ = createAndCommit(t, repo, "my_dir/file_in_dir.txt", []byte("content of file in dir"), "add dir and file") head, err := repo.Head() @@ -952,7 +960,7 @@ func TestGetHashForPathOrEmpty(t *testing.T) { if err != nil { t.Fatalf("repo.CommitObject failed: %v", err) } - return commit, "my_dir" + return localRepository, commit, "my_dir" }, wantHash: func(commit *object.Commit, path string) string { tree, err := commit.Tree() @@ -968,9 +976,9 @@ func TestGetHashForPathOrEmpty(t *testing.T) { }, { name: "non-existent file", - setup: func(t *testing.T) (*object.Commit, string) { - _, commit := setupInitialRepo(t) - return commit, "non_existent_file.txt" + setup: func(t *testing.T) (LocalRepository, *object.Commit, string) { + localRepository, commit := setupInitialRepo(t) + return localRepository, commit, "non_existent_file.txt" }, wantHash: func(commit *object.Commit, path string) string { return "" @@ -978,9 +986,9 @@ func TestGetHashForPathOrEmpty(t *testing.T) { }, { name: "non-existent directory", - setup: func(t *testing.T) (*object.Commit, string) { - _, commit := setupInitialRepo(t) - return commit, "non_existent_dir" + setup: func(t *testing.T) (LocalRepository, *object.Commit, string) { + localRepository, commit := setupInitialRepo(t) + return localRepository, commit, "non_existent_dir" }, wantHash: func(commit *object.Commit, path string) string { return "" @@ -988,8 +996,9 @@ func TestGetHashForPathOrEmpty(t *testing.T) { }, { name: "file in subdirectory", - setup: func(t *testing.T) (*object.Commit, string) { - repo, _ := setupInitialRepo(t) + setup: func(t *testing.T) (LocalRepository, *object.Commit, string) { + localRepository, _ := setupInitialRepo(t) + repo := localRepository.repo _ = createAndCommit(t, repo, "another_dir/sub_dir/nested_file.txt", []byte("nested content"), "add nested file") head, err := repo.Head() if err != nil { @@ -999,7 +1008,7 @@ func TestGetHashForPathOrEmpty(t *testing.T) { if err != nil { t.Fatalf("repo.CommitObject failed: %v", err) } - return commit, "another_dir/sub_dir/nested_file.txt" + return localRepository, commit, "another_dir/sub_dir/nested_file.txt" }, wantHash: func(commit *object.Commit, path string) string { tree, err := commit.Tree() @@ -1017,9 +1026,9 @@ func TestGetHashForPathOrEmpty(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() - commit, path := test.setup(t) + localRepository, commit, path := test.setup(t) - got, err := getHashForPathOrEmpty(commit, path) + got, err := localRepository.GetHashForPathOrEmpty(commit.Hash.String(), path) if (err != nil) != test.wantErr { t.Errorf("getHashForPathOrEmpty() error = %v, wantErr %v", err, test.wantErr) return @@ -1033,6 +1042,42 @@ func TestGetHashForPathOrEmpty(t *testing.T) { } } +// TestGetHashForPathOrEmptyBadCommitHash tests the one path not +// otherwise tested in TestGetHashForPathOrEmpty, where we can't +// get the commit for the hash. +func TestGetHashForPathOrEmptyBadCommitHash(t *testing.T) { + t.Helper() + repo, dir := initTestRepo(t) + localRepository := LocalRepository{ + Dir: dir, + repo: repo, + } + for _, test := range []struct { + name string + commitHash string + }{ + { + name: "empty hash", + commitHash: "", + }, + { + name: "invalid hash", + commitHash: "bad-hash", + }, + { + name: "hash not in repo", + commitHash: "d93e160f57f0a6eccd6e230dd40f465988bede63", + }, + } { + t.Run(test.name, func(t *testing.T) { + _, err := localRepository.GetHashForPathOrEmpty(test.commitHash, "path/to/file") + if err == nil { + t.Error("GetHashForPathOrEmpty() err = nil, should fail when an invalid or absent hash is provided") + } + }) + } +} + func TestChangedFilesInCommit(t *testing.T) { t.Parallel() r, commitHashes := setupRepoForChangedFilesTest(t) diff --git a/internal/librarian/flags.go b/internal/librarian/flags.go index 126f4e27f8..0f689d4e63 100644 --- a/internal/librarian/flags.go +++ b/internal/librarian/flags.go @@ -45,6 +45,12 @@ func addFlagCommit(fs *flag.FlagSet, cfg *config.Config) { a pull request. This flag is ignored if push is set to true.`) } +func addFlagGenerateUnchanged(fs *flag.FlagSet, cfg *config.Config) { + fs.BoolVar(&cfg.GenerateUnchanged, "generate-unchanged", false, + `If true, librarian generates libraries even if none of their associated APIs +have changed. This does not override generation being blocked by configuration.`) +} + func addFlagGitHubAPIEndpoint(fs *flag.FlagSet, cfg *config.Config) { fs.StringVar(&cfg.GitHubAPIEndpoint, "github-api-endpoint", "", `The GitHub API endpoint to use for all GitHub API operations. diff --git a/internal/librarian/generate_command.go b/internal/librarian/generate_command.go index ae3a6f9b29..bb74b7a9bb 100644 --- a/internal/librarian/generate_command.go +++ b/internal/librarian/generate_command.go @@ -32,21 +32,22 @@ const ( ) type generateRunner struct { - api string - branch string - build bool - commit bool - containerClient ContainerClient - ghClient GitHubClient - hostMount string - image string - library string - push bool - repo gitrepo.Repository - sourceRepo gitrepo.Repository - state *config.LibrarianState - librarianConfig *config.LibrarianConfig - workRoot string + api string + branch string + build bool + commit bool + generateUnchanged bool + containerClient ContainerClient + ghClient GitHubClient + hostMount string + image string + library string + push bool + repo gitrepo.Repository + sourceRepo gitrepo.Repository + state *config.LibrarianState + librarianConfig *config.LibrarianConfig + workRoot string } // generationStatus represents the result of a single library generation. @@ -62,21 +63,22 @@ func newGenerateRunner(cfg *config.Config) (*generateRunner, error) { return nil, err } return &generateRunner{ - api: cfg.API, - branch: cfg.Branch, - build: cfg.Build, - commit: cfg.Commit, - containerClient: runner.containerClient, - ghClient: runner.ghClient, - hostMount: cfg.HostMount, - image: runner.image, - library: cfg.Library, - push: cfg.Push, - repo: runner.repo, - sourceRepo: runner.sourceRepo, - state: runner.state, - librarianConfig: runner.librarianConfig, - workRoot: runner.workRoot, + api: cfg.API, + branch: cfg.Branch, + build: cfg.Build, + commit: cfg.Commit, + containerClient: runner.containerClient, + generateUnchanged: cfg.GenerateUnchanged, + ghClient: runner.ghClient, + hostMount: cfg.HostMount, + image: runner.image, + library: cfg.Library, + push: cfg.Push, + repo: runner.repo, + sourceRepo: runner.sourceRepo, + state: runner.state, + librarianConfig: runner.librarianConfig, + workRoot: runner.workRoot, }, nil } @@ -110,15 +112,21 @@ func (r *generateRunner) run(ctx context.Context) error { prType = status.prType } else { succeededGenerations := 0 - blockedGenerations := 0 + skippedGenerations := 0 for _, library := range r.state.Libraries { - if r.librarianConfig != nil { - libConfig := r.librarianConfig.LibraryConfigFor(library.ID) - if libConfig != nil && libConfig.GenerateBlocked { - slog.Info("library has generate_blocked, skipping", "id", library.ID) - blockedGenerations++ - continue - } + shouldGenerate, err := r.shouldGenerate(library) + if err != nil { + // We assume that the cause will have been logged in shouldGenerateLibrary. + // While this isn't strictly a failed generation, it's a library for which + // the generate command failed, so it's close enough. + failedLibraries = append(failedLibraries, library.ID) + failedGenerations++ + continue + } + if !shouldGenerate { + // We assume that the cause will have been logged in shouldGenerateLibrary. + skippedGenerations++ + continue } status, err := r.generateSingleLibrary(ctx, library.ID, outputDir) if err != nil { @@ -137,11 +145,11 @@ func (r *generateRunner) run(ctx context.Context) error { "generation statistics", "all", len(r.state.Libraries), "successes", succeededGenerations, - "blocked", blockedGenerations, + "skipped", skippedGenerations, "failures", failedGenerations) - if failedGenerations > 0 && failedGenerations+blockedGenerations == len(r.state.Libraries) { - return fmt.Errorf("all %d libraries failed to generate (blocked: %d)", - failedGenerations, blockedGenerations) + if failedGenerations > 0 && failedGenerations+skippedGenerations == len(r.state.Libraries) { + return fmt.Errorf("all %d libraries failed to generate (skipped: %d)", + failedGenerations, skippedGenerations) } } @@ -398,3 +406,62 @@ func setAllAPIStatus(state *config.LibrarianState, status string) { } } } + +// shouldGenerate determines whether a library should be generated by the generate +// command. It does *not* observe the -library or -api flag, as those are handled +// higher up in run. If this function returns a non-nil error or indicates that +// a library should not be generated, it always logs the cause; no further logging +// is required by the caller. +// +// This function observes the following logic: +// +// - If the library has a manual configuration which indicates it's blocked, the +// the library is skipped. +// - If the library has no APIs, it is skipped. +// - If r.generatedUnchanged is true (usually via the -generate-unchanged flag) +// then the library is generated. +// - If the library does not have a last_generated_commit then the library is generated. +// - If any API (as indicated by API.Path) has changed between the last_generated_commit +// and the HEAD commit of r.sourceRepo have changed, then the library is generated. +// - Otherwise, the library is skipped. +func (r *generateRunner) shouldGenerate(library *config.LibraryState) (bool, error) { + if r.librarianConfig != nil { + libConfig := r.librarianConfig.LibraryConfigFor(library.ID) + if libConfig != nil && libConfig.GenerateBlocked { + slog.Info("library has generate_blocked, skipping", "id", library.ID) + return false, nil + } + } + if len(library.APIs) == 0 { + slog.Info("library has no APIs, skipping", "id", library.ID) + return false, nil + } + // We don't need to find which APIs have changed if we've been asked to + // generate even unchanged ones. + if r.generateUnchanged { + return true, nil + } + // We can't find out which APIs have changed if we don't know at which + // commit the library was last generated. + if library.LastGeneratedCommit == "" { + return true, nil + } + headHash, err := r.sourceRepo.HeadHash() + if err != nil { + slog.Error("failed to get head hash for source repo", "library", library.ID) + return false, err + } + for _, api := range library.APIs { + oldHash, err := r.sourceRepo.GetHashForPathOrEmpty(library.LastGeneratedCommit, api.Path) + if err != nil { + slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", library.LastGeneratedCommit, "err", err) + return false, err + } + newHash, err := r.sourceRepo.GetHashForPathOrEmpty(headHash, api.Path) + if oldHash != newHash { + return true, nil + } + } + slog.Info("no APIs have changed; skipping", "library", library.ID) + return false, nil +} diff --git a/internal/librarian/generate_command_test.go b/internal/librarian/generate_command_test.go index de74170c76..774395f9a0 100644 --- a/internal/librarian/generate_command_test.go +++ b/internal/librarian/generate_command_test.go @@ -783,7 +783,7 @@ func TestGenerateScenarios(t *testing.T) { ghClient: &mockGitHubClient{}, build: true, wantErr: true, - wantErrMsg: "all 1 libraries failed to generate (blocked: 1)", + wantErrMsg: "all 1 libraries failed to generate (skipped: 1)", }, { name: "generate all, all fail should report error", @@ -1158,3 +1158,229 @@ func TestUpdateLastGeneratedCommitState(t *testing.T) { t.Errorf("updateState() got = %v, want %v", r.state.Libraries[0].LastGeneratedCommit, hash) } } + +func TestShouldGenerate(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name string + config *config.LibrarianConfig + state *config.LibrarianState + generateUnchanged bool + sourceRepo gitrepo.Repository + libraryIDToTest string + want bool + wantErr bool + }{ + // Tests that don't get as far as checking for hashes. + // (The mock repo will fail if we do get that far.) + { + name: "generation blocked", + config: &config.LibrarianConfig{ + Libraries: []*config.LibraryConfig{ + { + LibraryID: "TestLibrary", + GenerateBlocked: true, + }, + }, + }, + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedHash", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashError: errors.New("Shouldn't get as far as checking head"), + }, + libraryIDToTest: "TestLibrary", + want: false, + }, + { + name: "library has no APIs", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + // This may be present even if it's meaningless. + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashError: errors.New("Shouldn't get as far as checking head"), + }, + libraryIDToTest: "TestLibrary", + want: false, + }, + { + name: "generateUnchanged specified", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + generateUnchanged: true, + sourceRepo: &MockRepository{ + HeadHashError: errors.New("Shouldn't get as far as checking head"), + }, + libraryIDToTest: "TestLibrary", + want: true, + }, + { + name: "no LastGeneratedCommit", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashError: errors.New("Shouldn't get as far as checking head"), + }, + libraryIDToTest: "TestLibrary", + want: true, + }, + { + name: "error from HeadHash", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashError: errors.New("Can't get head commit"), + }, + libraryIDToTest: "TestLibrary", + wantErr: true, + }, + // Tests that do perform hash checking. + { + name: "error from GetHashForPathOrEmpty", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashValue: "HeadCommit", + GetHashForPathOrEmptyError: errors.New("Can't get hash for path"), + }, + libraryIDToTest: "TestLibrary", + wantErr: true, + }, + { + name: "config present but generation not blocked", + config: &config.LibrarianConfig{ + Libraries: []*config.LibraryConfig{ + { + LibraryID: "OtherLibrary", + GenerateBlocked: true, + }, + { + LibraryID: "TestLibrary", + // Just to have some reason to make it configured... + ReleaseBlocked: true, + }}, + }, + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashValue: "HeadCommit", + GetHashForPathOrEmptyValue: map[string]string{ + "LastGeneratedCommit:google/cloud/test": "hash1", + "HeadCommit:google/cloud/test": "hash2", + }, + }, + libraryIDToTest: "TestLibrary", + want: true, + }, + { + name: "API hasn't changed", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashValue: "HeadCommit", + GetHashForPathOrEmptyValue: map[string]string{ + "LastGeneratedCommit:google/cloud/test": "hash", + "HeadCommit:google/cloud/test": "hash", + }, + }, + libraryIDToTest: "TestLibrary", + want: false, + }, + { + name: "one API hasn't changed, one has", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{ + {Path: "google/cloud/test1"}, + {Path: "google/cloud/test2"}, + }, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashValue: "HeadCommit", + GetHashForPathOrEmptyValue: map[string]string{ + "LastGeneratedCommit:google/cloud/test1": "hash1", + "HeadCommit:google/cloud/test1": "hash1", + "LastGeneratedCommit:google/cloud/test2": "hash2a", + "HeadCommit:google/cloud/test2": "hash2b", + }, + }, + libraryIDToTest: "TestLibrary", + want: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + r := &generateRunner{ + generateUnchanged: test.generateUnchanged, + librarianConfig: test.config, + state: test.state, + sourceRepo: test.sourceRepo, + } + library := test.state.LibraryByID(test.libraryIDToTest) + got, err := r.shouldGenerate(library) + if test.wantErr != (err != nil) { + t.Fatalf("shouldGenerate() error = %v, wantErr %v", err, test.wantErr) + } + if got != test.want { + t.Errorf("shouldGenerate() got = %v, want %v", got, test.want) + } + }) + } +} diff --git a/internal/librarian/generate_pull_request.go b/internal/librarian/generate_pull_request.go index e258dd4f86..6935c01ad9 100644 --- a/internal/librarian/generate_pull_request.go +++ b/internal/librarian/generate_pull_request.go @@ -209,7 +209,7 @@ func findPiperIDFrom(commit *gitrepo.Commit, libraryID string) (string, error) { } // findLatestGenerationCommit returns the latest commit among the last generated -// commit of all the libraries. +// commit of all the libraries that have been generated. // A library is skipped if the last generated commit is empty. // // Note that it is possible that the returned commit is nil. @@ -219,7 +219,7 @@ func findLatestGenerationCommit(repo gitrepo.Repository, state *config.Librarian for _, library := range state.Libraries { commitHash, ok := idToCommits[library.ID] if !ok || commitHash == "" { - slog.Info("skip getting last generated commit", "library", library.ID) + slog.Debug("skip getting last generated commit", "library", library.ID) continue } commit, err := repo.GetCommit(commitHash) diff --git a/internal/librarian/librarian.go b/internal/librarian/librarian.go index c9ce8465ac..cfd9b73402 100644 --- a/internal/librarian/librarian.go +++ b/internal/librarian/librarian.go @@ -103,6 +103,7 @@ func newCmdGenerate() *cli.Command { addFlagAPI(cmdGenerate.Flags, cmdGenerate.Config) addFlagAPISource(cmdGenerate.Flags, cmdGenerate.Config) addFlagBuild(cmdGenerate.Flags, cmdGenerate.Config) + addFlagGenerateUnchanged(cmdGenerate.Flags, cmdGenerate.Config) addFlagHostMount(cmdGenerate.Flags, cmdGenerate.Config) addFlagImage(cmdGenerate.Flags, cmdGenerate.Config) addFlagLibrary(cmdGenerate.Flags, cmdGenerate.Config) diff --git a/internal/librarian/mocks_test.go b/internal/librarian/mocks_test.go index f8f07d0204..30482aabf8 100644 --- a/internal/librarian/mocks_test.go +++ b/internal/librarian/mocks_test.go @@ -346,6 +346,10 @@ type MockRepository struct { HeadHashError error CheckoutCalls int CheckoutError error + GetHashForPathOrEmptyError error + // GetHashForPathOrEmptyValue is a map where each key is of the form "commitHash:path", + // and the value is the hash to return. Every requested entry must be populated. + GetHashForPathOrEmptyValue map[string]string } func (m *MockRepository) HeadHash() (string, error) { @@ -511,3 +515,17 @@ func (m *mockImagesClient) FindLatest(ctx context.Context, imageName string) (st m.findLatestCalls++ return m.latestImage, m.err } + +func (m *MockRepository) GetHashForPathOrEmpty(commitHash, path string) (string, error) { + if m.GetHashForPathOrEmptyError != nil { + return "", m.GetHashForPathOrEmptyError + } + if m.GetHashForPathOrEmptyValue != nil { + key := commitHash + ":" + path + if hash, ok := m.GetHashForPathOrEmptyValue[key]; ok { + return hash, nil + } + + } + return "", errors.New("should not reach here") +} From d5619fe09b41d4a9dcc77945e60188a96e89e3bc Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Mon, 20 Oct 2025 10:06:31 +0000 Subject: [PATCH 2/5] chore: address review feedback --- internal/gitrepo/gitrepo_test.go | 1 - internal/librarian/generate_command.go | 4 ++++ internal/librarian/generate_command_test.go | 22 +++++++++++++++++++++ internal/librarian/mocks_test.go | 8 +++++++- 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/internal/gitrepo/gitrepo_test.go b/internal/gitrepo/gitrepo_test.go index 6d9b008776..59020eac75 100644 --- a/internal/gitrepo/gitrepo_test.go +++ b/internal/gitrepo/gitrepo_test.go @@ -1046,7 +1046,6 @@ func TestGetHashForPathOrEmpty(t *testing.T) { // otherwise tested in TestGetHashForPathOrEmpty, where we can't // get the commit for the hash. func TestGetHashForPathOrEmptyBadCommitHash(t *testing.T) { - t.Helper() repo, dir := initTestRepo(t) localRepository := LocalRepository{ Dir: dir, diff --git a/internal/librarian/generate_command.go b/internal/librarian/generate_command.go index bb74b7a9bb..8b1eba985b 100644 --- a/internal/librarian/generate_command.go +++ b/internal/librarian/generate_command.go @@ -458,6 +458,10 @@ func (r *generateRunner) shouldGenerate(library *config.LibraryState) (bool, err return false, err } newHash, err := r.sourceRepo.GetHashForPathOrEmpty(headHash, api.Path) + if err != nil { + slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", library.LastGeneratedCommit, "err", err) + return false, err + } if oldHash != newHash { return true, nil } diff --git a/internal/librarian/generate_command_test.go b/internal/librarian/generate_command_test.go index 774395f9a0..bb1648dfa4 100644 --- a/internal/librarian/generate_command_test.go +++ b/internal/librarian/generate_command_test.go @@ -1365,6 +1365,28 @@ func TestShouldGenerate(t *testing.T) { libraryIDToTest: "TestLibrary", want: true, }, + { + name: "second call to GetHashForPathOrEmpty fails", + state: &config.LibrarianState{ + Libraries: []*config.LibraryState{ + { + ID: "TestLibrary", + APIs: []*config.API{{Path: "google/cloud/test"}}, + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + sourceRepo: &MockRepository{ + HeadHashValue: "HeadCommit", + GetHashForPathOrEmptyValue: map[string]string{ + "LastGeneratedCommit:google/cloud/test": "hash", + // Entry which deliberately returns an error + "HeadCommit:google/cloud/test": "error", + }, + }, + libraryIDToTest: "TestLibrary", + wantErr: true, + }, } { t.Run(test.name, func(t *testing.T) { r := &generateRunner{ diff --git a/internal/librarian/mocks_test.go b/internal/librarian/mocks_test.go index 30482aabf8..bd3db35038 100644 --- a/internal/librarian/mocks_test.go +++ b/internal/librarian/mocks_test.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "path/filepath" @@ -349,6 +350,8 @@ type MockRepository struct { GetHashForPathOrEmptyError error // GetHashForPathOrEmptyValue is a map where each key is of the form "commitHash:path", // and the value is the hash to return. Every requested entry must be populated. + // If the value is "error", an error is returned instead. (This is useful when some + // calls must be successful, and others must fail.) GetHashForPathOrEmptyValue map[string]string } @@ -523,9 +526,12 @@ func (m *MockRepository) GetHashForPathOrEmpty(commitHash, path string) (string, if m.GetHashForPathOrEmptyValue != nil { key := commitHash + ":" + path if hash, ok := m.GetHashForPathOrEmptyValue[key]; ok { + if hash == "error" { + return "", errors.New("deliberate error from GetHashForPathOrEmpty") + } return hash, nil } } - return "", errors.New("should not reach here") + return "", fmt.Errorf("should not reach here: GetHashForPathOrEmpty called with unhandled input (commitHash: %q, path: %q)", commitHash, path) } From a6dcb5a2e6750dbfdc4001a5a02f6c6d7349979b Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Mon, 20 Oct 2025 10:10:15 +0000 Subject: [PATCH 3/5] chore: fix reported hash on failure --- internal/librarian/generate_command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/librarian/generate_command.go b/internal/librarian/generate_command.go index 8b1eba985b..4fe5d62226 100644 --- a/internal/librarian/generate_command.go +++ b/internal/librarian/generate_command.go @@ -459,7 +459,7 @@ func (r *generateRunner) shouldGenerate(library *config.LibraryState) (bool, err } newHash, err := r.sourceRepo.GetHashForPathOrEmpty(headHash, api.Path) if err != nil { - slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", library.LastGeneratedCommit, "err", err) + slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", headHash, "err", err) return false, err } if oldHash != newHash { From 70bb500505db2aad013bb9f4e6e9bcf94e306c56 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Mon, 20 Oct 2025 11:23:48 +0000 Subject: [PATCH 4/5] chore: add test for run() observing shouldGenerate failure --- internal/librarian/generate_command_test.go | 55 ++++++++++++++++----- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/internal/librarian/generate_command_test.go b/internal/librarian/generate_command_test.go index bb1648dfa4..2ba7e6259e 100644 --- a/internal/librarian/generate_command_test.go +++ b/internal/librarian/generate_command_test.go @@ -388,19 +388,20 @@ func TestRunConfigureCommand(t *testing.T) { func TestGenerateScenarios(t *testing.T) { t.Parallel() for _, test := range []struct { - name string - api string - library string - state *config.LibrarianState - librarianConfig *config.LibrarianConfig - container *mockContainerClient - ghClient GitHubClient - build bool - wantErr bool - wantErrMsg string - wantGenerateCalls int - wantBuildCalls int - wantConfigureCalls int + name string + api string + library string + state *config.LibrarianState + librarianConfig *config.LibrarianConfig + container *mockContainerClient + ghClient GitHubClient + build bool + forceShouldGenerateError bool + wantErr bool + wantErrMsg string + wantGenerateCalls int + wantBuildCalls int + wantConfigureCalls int }{ { name: "generate_single_library_including_initial_configuration", @@ -875,6 +876,28 @@ func TestGenerateScenarios(t *testing.T) { wantBuildCalls: 2, wantConfigureCalls: 0, }, + // We only have one library to generate, and we force shouldGenerate + // to fail by making the source repo's HeadHash function fail. + // As this ends up being all the libraries, the overall result is an error. + // (Forcing shouldGenerate to fail selectively would be very complicated.) + { + name: "shouldGenerate error", + state: &config.LibrarianState{ + Image: "gcr.io/test/image:v1.2.3", + Libraries: []*config.LibraryState{ + { + ID: "some-library", + APIs: []*config.API{{Path: "some/api"}}, + // We need the LastGeneratedCommit to force shouldGenerate + // to ask the source repo for the head hash. + LastGeneratedCommit: "LastGeneratedCommit", + }, + }, + }, + forceShouldGenerateError: true, + wantErr: true, + wantErrMsg: "all 1 libraries failed to generate", + }, } { t.Run(test.name, func(t *testing.T) { repo := newTestGitRepoWithState(t, test.state) @@ -911,6 +934,12 @@ func TestGenerateScenarios(t *testing.T) { t.Fatal(err) } + if test.forceShouldGenerateError { + r.sourceRepo = &MockRepository{ + HeadHashError: errors.New("fail"), + } + } + // Create a symlink in the output directory to trigger an error. if test.name == "symlink in output" { outputDir := filepath.Join(r.workRoot, "output") From 46651e72aab68c191b2df8a278af5f8666d668cf Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Tue, 21 Oct 2025 07:17:41 +0000 Subject: [PATCH 5/5] chore: address review comments --- internal/gitrepo/gitrepo.go | 16 ++--- internal/gitrepo/gitrepo_test.go | 22 +++---- internal/librarian/generate_command.go | 68 ++++++++++----------- internal/librarian/generate_command_test.go | 16 ++--- internal/librarian/mocks_test.go | 20 +++--- 5 files changed, 69 insertions(+), 73 deletions(-) diff --git a/internal/gitrepo/gitrepo.go b/internal/gitrepo/gitrepo.go index 34bde0aae9..ce62ce4c34 100644 --- a/internal/gitrepo/gitrepo.go +++ b/internal/gitrepo/gitrepo.go @@ -55,7 +55,7 @@ type Repository interface { CleanUntracked(paths []string) error pushRefSpec(refSpec string) error Checkout(commitHash string) error - GetHashForPathOrEmpty(commitHash, path string) (string, error) + GetHashForPath(commitHash, path string) (string, error) } const RootPath = "." @@ -420,20 +420,20 @@ func commitMatchesPath(path string, commit *object.Commit, parentCommit *object. if path == RootPath { return true, nil } - currentPathHash, err := getHashForPathOrEmpty(commit, path) + currentPathHash, err := getHashForPath(commit, path) if err != nil { return false, err } - parentPathHash, err := getHashForPathOrEmpty(parentCommit, path) + parentPathHash, err := getHashForPath(parentCommit, path) if err != nil { return false, err } return currentPathHash != parentPathHash, nil } -// getHashForPathOrEmpty returns the hash for a path at a given commit, or an +// getHashForPath returns the hash for a path at a given commit, or an // empty string if the path (file or directory) did not exist. -func getHashForPathOrEmpty(commit *object.Commit, path string) (string, error) { +func getHashForPath(commit *object.Commit, path string) (string, error) { tree, err := commit.Tree() if err != nil { return "", err @@ -665,18 +665,18 @@ func (r *LocalRepository) Checkout(commitSha string) error { }) } -// GetHashForPathOrEmpty returns a tree hash for the specified path, +// GetHashForPath returns a tree hash for the specified path, // at the given commit in this repository. If the path does not exist // at the commit, an empty string is returned rather than an error, // as the purpose of this function is to allow callers to determine changes // in the tree. (A path going from missing to anything else, or vice versa, // indicates a change. A path being missing at two different commits is not a change.) -func (r *LocalRepository) GetHashForPathOrEmpty(commitHash, path string) (string, error) { +func (r *LocalRepository) GetHashForPath(commitHash, path string) (string, error) { // This public function just delegates to the internal function that uses a Commit // object instead of the hash. commit, err := r.repo.CommitObject(plumbing.NewHash(commitHash)) if err != nil { return "", err } - return getHashForPathOrEmpty(commit, path) + return getHashForPath(commit, path) } diff --git a/internal/gitrepo/gitrepo_test.go b/internal/gitrepo/gitrepo_test.go index 59020eac75..555d5ea11f 100644 --- a/internal/gitrepo/gitrepo_test.go +++ b/internal/gitrepo/gitrepo_test.go @@ -904,10 +904,10 @@ func TestGetDir(t *testing.T) { } } -// TestGetHashForPathOrEmpty tests the internal getHashForPathOrEmpty, but -// via the public GetHashForPathOrEmpty function which accepts a commit hash +// TestGetHashForPath tests the internal getHashForPath, but +// via the public GetHashForPath function which accepts a commit hash // instead of a Commit object, to avoid duplicate testing. -func TestGetHashForPathOrEmpty(t *testing.T) { +func TestGetHashForPath(t *testing.T) { t.Parallel() setupInitialRepo := func(t *testing.T) (LocalRepository, *object.Commit) { @@ -1028,24 +1028,24 @@ func TestGetHashForPathOrEmpty(t *testing.T) { localRepository, commit, path := test.setup(t) - got, err := localRepository.GetHashForPathOrEmpty(commit.Hash.String(), path) + got, err := localRepository.GetHashForPath(commit.Hash.String(), path) if (err != nil) != test.wantErr { - t.Errorf("getHashForPathOrEmpty() error = %v, wantErr %v", err, test.wantErr) + t.Errorf("getHashForPath() error = %v, wantErr %v", err, test.wantErr) return } wantHash := test.wantHash(commit, path) if diff := cmp.Diff(wantHash, got); diff != "" { - t.Errorf("getHashForPathOrEmpty() mismatch (-want +got):\n%s", diff) + t.Errorf("getHashForPath() mismatch (-want +got):\n%s", diff) } }) } } -// TestGetHashForPathOrEmptyBadCommitHash tests the one path not -// otherwise tested in TestGetHashForPathOrEmpty, where we can't +// TestGetHashForPathBadCommitHash tests the one path not +// otherwise tested in TestGetHashForPath, where we can't // get the commit for the hash. -func TestGetHashForPathOrEmptyBadCommitHash(t *testing.T) { +func TestGetHashForPathBadCommitHash(t *testing.T) { repo, dir := initTestRepo(t) localRepository := LocalRepository{ Dir: dir, @@ -1069,9 +1069,9 @@ func TestGetHashForPathOrEmptyBadCommitHash(t *testing.T) { }, } { t.Run(test.name, func(t *testing.T) { - _, err := localRepository.GetHashForPathOrEmpty(test.commitHash, "path/to/file") + _, err := localRepository.GetHashForPath(test.commitHash, "path/to/file") if err == nil { - t.Error("GetHashForPathOrEmpty() err = nil, should fail when an invalid or absent hash is provided") + t.Error("GetHashForPath() err = nil, should fail when an invalid or absent hash is provided") } }) } diff --git a/internal/librarian/generate_command.go b/internal/librarian/generate_command.go index 4fe5d62226..948bfd9cc3 100644 --- a/internal/librarian/generate_command.go +++ b/internal/librarian/generate_command.go @@ -97,7 +97,6 @@ func (r *generateRunner) run(ctx context.Context) error { // generation since we need these commits to create pull request body. idToCommits := make(map[string]string) var failedLibraries []string - failedGenerations := 0 prType := pullRequestGenerate if r.api != "" || r.library != "" { libraryID := r.library @@ -111,16 +110,15 @@ func (r *generateRunner) run(ctx context.Context) error { idToCommits[libraryID] = status.oldCommit prType = status.prType } else { - succeededGenerations := 0 - skippedGenerations := 0 + var succeededGenerations int + var skippedGenerations int for _, library := range r.state.Libraries { shouldGenerate, err := r.shouldGenerate(library) if err != nil { - // We assume that the cause will have been logged in shouldGenerateLibrary. + slog.Error("failed to determine whether or not to generate library", "id", library.ID, "err", err) // While this isn't strictly a failed generation, it's a library for which // the generate command failed, so it's close enough. failedLibraries = append(failedLibraries, library.ID) - failedGenerations++ continue } if !shouldGenerate { @@ -132,7 +130,6 @@ func (r *generateRunner) run(ctx context.Context) error { 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. @@ -146,10 +143,10 @@ func (r *generateRunner) run(ctx context.Context) error { "all", len(r.state.Libraries), "successes", succeededGenerations, "skipped", skippedGenerations, - "failures", failedGenerations) - if failedGenerations > 0 && failedGenerations+skippedGenerations == len(r.state.Libraries) { + "failures", len(failedLibraries)) + if len(failedLibraries) > 0 && len(failedLibraries)+skippedGenerations == len(r.state.Libraries) { return fmt.Errorf("all %d libraries failed to generate (skipped: %d)", - failedGenerations, skippedGenerations) + len(failedLibraries), skippedGenerations) } } @@ -197,7 +194,7 @@ func (r *generateRunner) run(ctx context.Context) error { workRoot: r.workRoot, api: r.api, library: r.library, - failedGenerations: failedGenerations, + failedGenerations: len(failedLibraries), prBodyBuilder: prBodyBuilder, } @@ -409,22 +406,15 @@ func setAllAPIStatus(state *config.LibrarianState, status string) { // shouldGenerate determines whether a library should be generated by the generate // command. It does *not* observe the -library or -api flag, as those are handled -// higher up in run. If this function returns a non-nil error or indicates that -// a library should not be generated, it always logs the cause; no further logging -// is required by the caller. -// -// This function observes the following logic: +// higher up in run. If this function returns false (with a nil error), it always +// logs why the library was skipped. // -// - If the library has a manual configuration which indicates it's blocked, the -// the library is skipped. -// - If the library has no APIs, it is skipped. -// - If r.generatedUnchanged is true (usually via the -generate-unchanged flag) -// then the library is generated. -// - If the library does not have a last_generated_commit then the library is generated. -// - If any API (as indicated by API.Path) has changed between the last_generated_commit -// and the HEAD commit of r.sourceRepo have changed, then the library is generated. -// - Otherwise, the library is skipped. +// The decision of whether or not a library should be generated is relatively complex, +// and should be kept centrally in this function, with a comment for each path in the flow +// for clarity. func (r *generateRunner) shouldGenerate(library *config.LibraryState) (bool, error) { + // If the library has a manual configuration which indicates generation is blocked, + // the library is skipped. if r.librarianConfig != nil { libConfig := r.librarianConfig.LibraryConfigFor(library.ID) if libConfig != nil && libConfig.GenerateBlocked { @@ -432,35 +422,41 @@ func (r *generateRunner) shouldGenerate(library *config.LibraryState) (bool, err return false, nil } } + + // If the library has no APIs, it is skipped. if len(library.APIs) == 0 { slog.Info("library has no APIs, skipping", "id", library.ID) return false, nil } - // We don't need to find which APIs have changed if we've been asked to - // generate even unchanged ones. + + // If we've been asked to generate libraries even with unchanged APIs, + // we don't need to check whether any have changed: we should definitely generate. if r.generateUnchanged { return true, nil } - // We can't find out which APIs have changed if we don't know at which - // commit the library was last generated. + + // If we don't know the last commit at which the library was generated, + // we can't tell whether or not it's changed, so we always generate. if library.LastGeneratedCommit == "" { return true, nil } + + // Most common case: a non-generation-blocked library with APIs, and without the + // -generate-unchanged flag. Check each API to see whether anything under API.Path + // has changed between the last_generated_commit and the HEAD commit of r.sourceRepo. + // If any API has changed, the library is generated - otherwise it's skipped. headHash, err := r.sourceRepo.HeadHash() if err != nil { - slog.Error("failed to get head hash for source repo", "library", library.ID) - return false, err + return false, fmt.Errorf("failed to get head hash for source repo: %v", err) } for _, api := range library.APIs { - oldHash, err := r.sourceRepo.GetHashForPathOrEmpty(library.LastGeneratedCommit, api.Path) + oldHash, err := r.sourceRepo.GetHashForPath(library.LastGeneratedCommit, api.Path) if err != nil { - slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", library.LastGeneratedCommit, "err", err) - return false, err + return false, fmt.Errorf("failed to get hash for path %v at commit %v: %v", api.Path, library.LastGeneratedCommit, err) } - newHash, err := r.sourceRepo.GetHashForPathOrEmpty(headHash, api.Path) + newHash, err := r.sourceRepo.GetHashForPath(headHash, api.Path) if err != nil { - slog.Error("failed to get hash for API path", "library", library.ID, "path", api.Path, "commit", headHash, "err", err) - return false, err + return false, fmt.Errorf("failed to get hash for path %v at commit %v: %v", api.Path, headHash, err) } if oldHash != newHash { return true, nil diff --git a/internal/librarian/generate_command_test.go b/internal/librarian/generate_command_test.go index 2ba7e6259e..c7798e1d2d 100644 --- a/internal/librarian/generate_command_test.go +++ b/internal/librarian/generate_command_test.go @@ -1297,7 +1297,7 @@ func TestShouldGenerate(t *testing.T) { }, // Tests that do perform hash checking. { - name: "error from GetHashForPathOrEmpty", + name: "error from GetHashForPath", state: &config.LibrarianState{ Libraries: []*config.LibraryState{ { @@ -1308,8 +1308,8 @@ func TestShouldGenerate(t *testing.T) { }, }, sourceRepo: &MockRepository{ - HeadHashValue: "HeadCommit", - GetHashForPathOrEmptyError: errors.New("Can't get hash for path"), + HeadHashValue: "HeadCommit", + GetHashForPathError: errors.New("Can't get hash for path"), }, libraryIDToTest: "TestLibrary", wantErr: true, @@ -1339,7 +1339,7 @@ func TestShouldGenerate(t *testing.T) { }, sourceRepo: &MockRepository{ HeadHashValue: "HeadCommit", - GetHashForPathOrEmptyValue: map[string]string{ + GetHashForPathValue: map[string]string{ "LastGeneratedCommit:google/cloud/test": "hash1", "HeadCommit:google/cloud/test": "hash2", }, @@ -1360,7 +1360,7 @@ func TestShouldGenerate(t *testing.T) { }, sourceRepo: &MockRepository{ HeadHashValue: "HeadCommit", - GetHashForPathOrEmptyValue: map[string]string{ + GetHashForPathValue: map[string]string{ "LastGeneratedCommit:google/cloud/test": "hash", "HeadCommit:google/cloud/test": "hash", }, @@ -1384,7 +1384,7 @@ func TestShouldGenerate(t *testing.T) { }, sourceRepo: &MockRepository{ HeadHashValue: "HeadCommit", - GetHashForPathOrEmptyValue: map[string]string{ + GetHashForPathValue: map[string]string{ "LastGeneratedCommit:google/cloud/test1": "hash1", "HeadCommit:google/cloud/test1": "hash1", "LastGeneratedCommit:google/cloud/test2": "hash2a", @@ -1395,7 +1395,7 @@ func TestShouldGenerate(t *testing.T) { want: true, }, { - name: "second call to GetHashForPathOrEmpty fails", + name: "second call to GetHashForPath fails", state: &config.LibrarianState{ Libraries: []*config.LibraryState{ { @@ -1407,7 +1407,7 @@ func TestShouldGenerate(t *testing.T) { }, sourceRepo: &MockRepository{ HeadHashValue: "HeadCommit", - GetHashForPathOrEmptyValue: map[string]string{ + GetHashForPathValue: map[string]string{ "LastGeneratedCommit:google/cloud/test": "hash", // Entry which deliberately returns an error "HeadCommit:google/cloud/test": "error", diff --git a/internal/librarian/mocks_test.go b/internal/librarian/mocks_test.go index bd3db35038..077a422bd1 100644 --- a/internal/librarian/mocks_test.go +++ b/internal/librarian/mocks_test.go @@ -347,12 +347,12 @@ type MockRepository struct { HeadHashError error CheckoutCalls int CheckoutError error - GetHashForPathOrEmptyError error - // GetHashForPathOrEmptyValue is a map where each key is of the form "commitHash:path", + GetHashForPathError error + // GetHashForPathValue is a map where each key is of the form "commitHash:path", // and the value is the hash to return. Every requested entry must be populated. // If the value is "error", an error is returned instead. (This is useful when some // calls must be successful, and others must fail.) - GetHashForPathOrEmptyValue map[string]string + GetHashForPathValue map[string]string } func (m *MockRepository) HeadHash() (string, error) { @@ -519,19 +519,19 @@ func (m *mockImagesClient) FindLatest(ctx context.Context, imageName string) (st return m.latestImage, m.err } -func (m *MockRepository) GetHashForPathOrEmpty(commitHash, path string) (string, error) { - if m.GetHashForPathOrEmptyError != nil { - return "", m.GetHashForPathOrEmptyError +func (m *MockRepository) GetHashForPath(commitHash, path string) (string, error) { + if m.GetHashForPathError != nil { + return "", m.GetHashForPathError } - if m.GetHashForPathOrEmptyValue != nil { + if m.GetHashForPathValue != nil { key := commitHash + ":" + path - if hash, ok := m.GetHashForPathOrEmptyValue[key]; ok { + if hash, ok := m.GetHashForPathValue[key]; ok { if hash == "error" { - return "", errors.New("deliberate error from GetHashForPathOrEmpty") + return "", errors.New("deliberate error from GetHashForPath") } return hash, nil } } - return "", fmt.Errorf("should not reach here: GetHashForPathOrEmpty called with unhandled input (commitHash: %q, path: %q)", commitHash, path) + return "", fmt.Errorf("should not reach here: GetHashForPath called with unhandled input (commitHash: %q, path: %q)", commitHash, path) }