Skip to content

Commit 0adeeac

Browse files
jskeetjskeet
authored
fix(internal/librarian): avoid work duplication when finding changes (#2558)
This changes how we format a generation PR. Instead of finding which files have changed in the language repo (an operation which doesn't depend on the library) once for each library, we do so once at the start. We can then skip libraries which haven't changed, without even looking at the API source repo. Fixes #2557
1 parent 895dac9 commit 0adeeac

File tree

4 files changed

+148
-115
lines changed

4 files changed

+148
-115
lines changed

‎internal/librarian/commit_version_analyzer.go‎

Lines changed: 6 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func shouldIncludeForRelease(files, sourceRoots, excludePaths []string) bool {
5858
// getConventionalCommitsSinceLastGeneration returns all conventional commits for
5959
// all API paths in given library since the last generation. The repo input should
6060
// be the googleapis source repo.
61-
func getConventionalCommitsSinceLastGeneration(sourceRepo, languageRepo gitrepo.Repository, library *config.LibraryState, lastGenCommit string) ([]*gitrepo.ConventionalCommit, error) {
61+
func getConventionalCommitsSinceLastGeneration(sourceRepo gitrepo.Repository, library *config.LibraryState, lastGenCommit string) ([]*gitrepo.ConventionalCommit, error) {
6262
if lastGenCommit == "" {
6363
slog.Info("the last generation commit is empty, skip fetching conventional commits", "library", library.ID)
6464
return make([]*gitrepo.ConventionalCommit, 0), nil
@@ -74,59 +74,30 @@ func getConventionalCommitsSinceLastGeneration(sourceRepo, languageRepo gitrepo.
7474
return nil, fmt.Errorf("failed to get commits for library %s at commit %s: %w", library.ID, lastGenCommit, err)
7575
}
7676

77-
var languageRepoFiles []string
78-
if ok, _ := languageRepo.IsClean(); ok {
79-
headHash, err := languageRepo.HeadHash()
80-
if err != nil {
81-
return nil, err
82-
}
83-
languageRepoFiles, err = languageRepo.ChangedFilesInCommit(headHash)
84-
if err != nil {
85-
return nil, err
86-
}
87-
} else {
88-
// The commit or push flag is not set, get all locally changed files.
89-
languageRepoFiles, err = languageRepo.ChangedFiles()
90-
if err != nil {
91-
return nil, err
92-
}
93-
}
94-
9577
// checks that the files in the commit are in the api paths for the source repo.
9678
// The generation change is for changes in the source repo and NOT the language repo.
9779
shouldIncludeFiles := func(sourceFiles []string) bool {
98-
return shouldIncludeForGeneration(sourceFiles, languageRepoFiles, library)
80+
return shouldIncludeForGeneration(sourceFiles, library)
9981
}
10082

10183
return convertToConventionalCommits(sourceRepo, library, sourceCommits, shouldIncludeFiles)
10284
}
10385

10486
// shouldIncludeForGeneration determines if a commit should be included in generation.
10587
// It returns true if there is at least one file in the commit that is under the
106-
// library's API(s) path (a library could have multiple APIs) and has local
107-
// changes associated with it.
108-
func shouldIncludeForGeneration(sourceFiles, languageRepoFiles []string, library *config.LibraryState) bool {
88+
// library's API(s) path (a library could have multiple APIs).
89+
func shouldIncludeForGeneration(sourceFiles []string, library *config.LibraryState) bool {
10990
var apiPaths []string
11091
for _, api := range library.APIs {
11192
apiPaths = append(apiPaths, api.Path)
11293
}
11394

114-
var sourceFilesInPath bool
11595
for _, file := range sourceFiles {
11696
if isUnderAnyPath(file, apiPaths) {
117-
sourceFilesInPath = true
118-
break
119-
}
120-
}
121-
122-
var languageRepoFilesInPath bool
123-
for _, file := range languageRepoFiles {
124-
if isUnderAnyPath(file, library.SourceRoots) && !isUnderAnyPath(file, library.ReleaseExcludePaths) {
125-
languageRepoFilesInPath = true
126-
break
97+
return true
12798
}
12899
}
129-
return sourceFilesInPath && languageRepoFilesInPath
100+
return false
130101
}
131102

132103
// libraryFilter filters a list of conventional commits by library ID.

‎internal/librarian/commit_version_analyzer_test.go‎

Lines changed: 20 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -145,106 +145,47 @@ func TestShouldIncludeForRelease(t *testing.T) {
145145
func TestShouldIncludeForGeneration(t *testing.T) {
146146
t.Parallel()
147147
cases := []struct {
148-
name string
149-
sourceFiles []string
150-
languageRepoFiles []string
151-
library *config.LibraryState
152-
want bool
148+
name string
149+
sourceFiles []string
150+
library *config.LibraryState
151+
want bool
153152
}{
154153
{
155-
name: "include: source and language files in path",
156-
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto"},
157-
languageRepoFiles: []string{"google/cloud/aiplatform/featurestore_client.go"},
154+
name: "include: source files in path",
155+
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto"},
158156
library: &config.LibraryState{
159-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
160-
SourceRoots: []string{"google/cloud/aiplatform"},
157+
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
161158
},
162159
want: true,
163160
},
164161
{
165-
name: "exclude: no source files in path",
166-
sourceFiles: []string{"google/cloud/vision/v1/image_annotator.proto"},
167-
languageRepoFiles: []string{"google/cloud/aiplatform/featurestore_client.go"},
168-
library: &config.LibraryState{
169-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
170-
SourceRoots: []string{"google/cloud/aiplatform"},
171-
},
172-
want: false,
173-
},
174-
{
175-
name: "exclude: no language repo files in path",
176-
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto"},
177-
languageRepoFiles: []string{"google/cloud/vision/image_annotator_client.go"},
178-
library: &config.LibraryState{
179-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
180-
SourceRoots: []string{"google/cloud/aiplatform"},
181-
},
182-
want: false,
183-
},
184-
{
185-
name: "exclude: language repo file in excluded path",
186-
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto"},
187-
languageRepoFiles: []string{"google/cloud/aiplatform/internal/generated/featurestore_client.go"},
162+
name: "exclude: no source files in path",
163+
sourceFiles: []string{"google/cloud/vision/v1/image_annotator.proto"},
188164
library: &config.LibraryState{
189-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
190-
SourceRoots: []string{"google/cloud/aiplatform"},
191-
ReleaseExcludePaths: []string{"google/cloud/aiplatform/internal"},
165+
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
192166
},
193167
want: false,
194168
},
195169
{
196-
name: "include: multiple files, some matching",
197-
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto", "unrelated/file"},
198-
languageRepoFiles: []string{"google/cloud/aiplatform/featurestore_client.go", "unrelated/file"},
170+
name: "include: multiple files, some matching",
171+
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto", "unrelated/file"},
199172
library: &config.LibraryState{
200-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
201-
SourceRoots: []string{"google/cloud/aiplatform"},
173+
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
202174
},
203175
want: true,
204176
},
205177
{
206-
name: "include: multiple source roots and APIs",
207-
sourceFiles: []string{"google/cloud/vision/v1/image_annotator.proto"},
208-
languageRepoFiles: []string{"google/cloud/aiplatform/featurestore_client.go"},
178+
name: "include: multiple APIs",
179+
sourceFiles: []string{"google/cloud/vision/v1/image_annotator.proto"},
209180
library: &config.LibraryState{
210-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}, {Path: "google/cloud/vision/v1"}},
211-
SourceRoots: []string{"google/cloud/aiplatform", "google/cloud/vision"},
181+
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}, {Path: "google/cloud/vision/v1"}},
212182
},
213183
want: true,
214184
},
215185
{
216-
name: "include: source root all files",
217-
sourceFiles: []string{"google/cloud/vision/v1/image_annotator.proto"},
218-
languageRepoFiles: []string{"google/cloud/vision/v1/image_annotator.go"},
219-
library: &config.LibraryState{
220-
APIs: []*config.API{{Path: "google/cloud/vision/v1"}},
221-
SourceRoots: []string{"."},
222-
},
223-
want: true,
224-
},
225-
{
226-
name: "exclude: empty source files",
227-
languageRepoFiles: []string{"google/cloud/aiplatform/featurestore_client.go"},
228-
library: &config.LibraryState{
229-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
230-
SourceRoots: []string{"google/cloud/aiplatform"},
231-
},
232-
want: false,
233-
},
234-
{
235-
name: "exclude: empty language repo files",
236-
sourceFiles: []string{"google/cloud/aiplatform/v1/featurestore_service.proto"},
237-
library: &config.LibraryState{
238-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
239-
SourceRoots: []string{"google/cloud/aiplatform"},
240-
},
241-
want: false,
242-
},
243-
{
244-
name: "exclude: both file lists empty",
186+
name: "exclude: empty source files",
245187
library: &config.LibraryState{
246-
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
247-
SourceRoots: []string{"google/cloud/aiplatform"},
188+
APIs: []*config.API{{Path: "google/cloud/aiplatform/v1"}},
248189
},
249190
want: false,
250191
},
@@ -253,7 +194,7 @@ func TestShouldIncludeForGeneration(t *testing.T) {
253194
for _, tc := range cases {
254195
t.Run(tc.name, func(t *testing.T) {
255196
t.Parallel()
256-
got := shouldIncludeForGeneration(tc.sourceFiles, tc.languageRepoFiles, tc.library)
197+
got := shouldIncludeForGeneration(tc.sourceFiles, tc.library)
257198
if got != tc.want {
258199
t.Errorf("shouldIncludeForGeneration() = %v, want %v", got, tc.want)
259200
}
@@ -593,7 +534,7 @@ func TestGetConventionalCommitsSinceLastGeneration(t *testing.T) {
593534
},
594535
} {
595536
t.Run(test.name, func(t *testing.T) {
596-
got, err := getConventionalCommitsSinceLastGeneration(test.sourceRepo, test.languageRepo, test.library, "1234")
537+
got, err := getConventionalCommitsSinceLastGeneration(test.sourceRepo, test.library, "1234")
597538
if test.wantErr {
598539
if err == nil {
599540
t.Fatal("getConventionalCommitsSinceLastGeneration() should have failed")

‎internal/librarian/release_notes.go‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,13 +156,22 @@ type commitSection struct {
156156
// Only consider libraries whose ID appears in idToCommits.
157157
func formatGenerationPRBody(sourceRepo, languageRepo gitrepo.Repository, state *config.LibrarianState, idToCommits map[string]string, failedLibraries []string) (string, error) {
158158
var allCommits []*gitrepo.ConventionalCommit
159+
languageRepoChanges, err := languageRepoChangedFiles(languageRepo)
160+
if err != nil {
161+
return "", fmt.Errorf("failed to fetch changes in language repo: %w", err)
162+
}
159163
for _, library := range state.Libraries {
160164
lastGenCommit, ok := idToCommits[library.ID]
161165
if !ok {
162166
continue
163167
}
168+
// If nothing has changed that would be significant in a release for this library,
169+
// we don't look at the API changes either.
170+
if !shouldIncludeForRelease(languageRepoChanges, library.SourceRoots, library.ReleaseExcludePaths) {
171+
continue
172+
}
164173

165-
commits, err := getConventionalCommitsSinceLastGeneration(sourceRepo, languageRepo, library, lastGenCommit)
174+
commits, err := getConventionalCommitsSinceLastGeneration(sourceRepo, library, lastGenCommit)
166175
if err != nil {
167176
return "", fmt.Errorf("failed to fetch conventional commits for library, %s: %w", library.ID, err)
168177
}
@@ -341,3 +350,22 @@ func formatLibraryReleaseNotes(library *config.LibraryState, ghRepo *github.Repo
341350

342351
return section
343352
}
353+
354+
// languageRepoChangedFiles returns the paths of files changed in the repo as part
355+
// of the current librarian run - either in the head commit if the repo is clean,
356+
// or the outstanding changes otherwise.
357+
func languageRepoChangedFiles(languageRepo gitrepo.Repository) ([]string, error) {
358+
clean, err := languageRepo.IsClean()
359+
if err != nil {
360+
return nil, err
361+
}
362+
if clean {
363+
headHash, err := languageRepo.HeadHash()
364+
if err != nil {
365+
return nil, err
366+
}
367+
return languageRepo.ChangedFilesInCommit(headHash)
368+
}
369+
// The commit or push flag is not set, get all locally changed files.
370+
return languageRepo.ChangedFiles()
371+
}

‎internal/librarian/release_notes_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,27 @@ Language Image: %s`,
483483
},
484484
want: "No commit is found since last generation",
485485
},
486+
{
487+
name: "failed to get language repo changes commits",
488+
state: &config.LibrarianState{
489+
Image: "go:1.21",
490+
Libraries: []*config.LibraryState{
491+
{
492+
ID: "one-library",
493+
SourceRoots: []string{"path/to"},
494+
},
495+
},
496+
},
497+
sourceRepo: &MockRepository{},
498+
languageRepo: &MockRepository{
499+
IsCleanError: errors.New("simulated error"),
500+
},
501+
idToCommits: map[string]string{
502+
"one-library": "1234567890",
503+
},
504+
wantErr: true,
505+
wantErrPhrase: "failed to fetch changes in language repo",
506+
},
486507
{
487508
name: "failed to get conventional commits",
488509
state: &config.LibrarianState{
@@ -498,6 +519,7 @@ Language Image: %s`,
498519
GetCommitsForPathsSinceLastGenError: errors.New("simulated error"),
499520
},
500521
languageRepo: &MockRepository{
522+
IsCleanValue: true,
501523
HeadHashValue: "5678",
502524
ChangedFilesInCommitValue: []string{"path/to/a.go"},
503525
},
@@ -1054,3 +1076,74 @@ Language Image: go:1.21
10541076
})
10551077
}
10561078
}
1079+
1080+
func TestLanguageRepoChangedFiles(t *testing.T) {
1081+
for _, test := range []struct {
1082+
name string
1083+
repo gitrepo.Repository
1084+
want []string
1085+
wantErr bool
1086+
}{
1087+
{
1088+
name: "IsClean fails",
1089+
repo: &MockRepository{
1090+
IsCleanError: fmt.Errorf("mock failure from IsClean"),
1091+
},
1092+
wantErr: true,
1093+
},
1094+
{
1095+
name: "clean, HeadHash fails",
1096+
repo: &MockRepository{
1097+
IsCleanValue: true,
1098+
HeadHashError: fmt.Errorf("mock failure from HeadHash"),
1099+
},
1100+
wantErr: true,
1101+
},
1102+
{
1103+
name: "clean, ChangedFilesInCommit fails",
1104+
repo: &MockRepository{
1105+
IsCleanValue: true,
1106+
HeadHashValue: "1234",
1107+
ChangedFilesInCommitError: fmt.Errorf("mock failure from ChangedFilesInCommit"),
1108+
},
1109+
wantErr: true,
1110+
},
1111+
{
1112+
name: "dirty, ChangedFiles fails",
1113+
repo: &MockRepository{
1114+
ChangedFilesError: fmt.Errorf("mock failure from ChangedFiles"),
1115+
},
1116+
wantErr: true,
1117+
},
1118+
{
1119+
name: "clean success",
1120+
repo: &MockRepository{
1121+
IsCleanValue: true,
1122+
HeadHashValue: "1234",
1123+
ChangedFilesInCommitValueByHash: map[string][]string{
1124+
"abcd": []string{"a/b/c", "d/e/f"},
1125+
"1234": []string{"g/h/i", "j/k/l"},
1126+
},
1127+
},
1128+
want: []string{"g/h/i", "j/k/l"},
1129+
},
1130+
{
1131+
name: "dirty success",
1132+
repo: &MockRepository{
1133+
ChangedFilesValue: []string{"a/b/c", "d/e/f"},
1134+
},
1135+
want: []string{"a/b/c", "d/e/f"},
1136+
},
1137+
} {
1138+
t.Run(test.name, func(t *testing.T) {
1139+
got, err := languageRepoChangedFiles(test.repo)
1140+
if (err != nil) != test.wantErr {
1141+
t.Errorf("languageRepoChangedFiles() error = %v, wantErr %v", err, test.wantErr)
1142+
return
1143+
}
1144+
if diff := cmp.Diff(test.want, got); diff != "" {
1145+
t.Errorf("mismatch (-want +got):\n%s", diff)
1146+
}
1147+
})
1148+
}
1149+
}

0 commit comments

Comments
 (0)