Skip to content

Commit 2edd323

Browse files
zhumin8zhumin8
authored
fix(internal/librarian): fix release_exclude_paths logic (#2329)
Change exclude logic from "If all files from commit belong to one of the paths it will be skipped." to "Files that belong to one of the paths in the exclusion list should not count towards a commit for the library in which they're excluded". inverted shouldExclude() to shouldInclude() for clarity, added test case. Some tests in release_init_test.go and release_notes_test.go needed changes because now SourceRoots are needed in exclude logic and it cannot be empty (should not be empty anyway according to schema), added to mocks. Fixes #2307
1 parent 2ff8248 commit 2edd323

File tree

8 files changed

+179
-87
lines changed

8 files changed

+179
-87
lines changed

‎doc/state-schema.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ Each object in the `libraries` list represents a single library and has the foll
2424
| `source_roots` | list | A list of directories in the language repository where Librarian contributes code. | Yes | Must not be empty, and each path must be a valid directory path. |
2525
| `preserve_regex` | list | A list of regular expressions for files and directories to preserve during the copy and remove process. | No | Each entry must be a valid regular expression. |
2626
| `remove_regex` | list | A list of regular expressions for files and directories to remove before copying generated code. If not set, this defaults to the `source_roots`. A more specific `preserve_regex` takes precedence. | No | Each entry must be a valid regular expression. |
27-
| `release_exclude_paths` | list | A list of directories to exclude from the release. | No | Each entry must be a valid directory path. |
27+
| `release_exclude_paths` | list | A list of paths to exclude from the release. Files matching these paths will not be considered part of a commit for this library. | No | Each entry must be a valid directory file/path. |
2828
| `tag_format` | string | A format string for the release tag. The supported placeholders are `{id}` and `{version}`. | No | Must contain `{version}` and may optionally contain `{id}`. No other placeholders are allowed. |
2929

3030
## `apis` Object

‎internal/config/librarian_config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ var validPermissions = map[string]bool{
5454
func (g *LibrarianConfig) Validate() error {
5555
for i, globalFile := range g.GlobalFilesAllowlist {
5656
path, permissions := globalFile.Path, globalFile.Permissions
57-
if !isValidDirPath(path) {
57+
if !isValidRelativePath(path) {
5858
return fmt.Errorf("invalid global file path at index %d: %q", i, path)
5959
}
6060
if _, ok := validPermissions[permissions]; !ok {

‎internal/config/state.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,8 @@ type LibraryState struct {
126126
// If not set, this defaults to the `source_roots`.
127127
// A more specific `preserve_regex` takes precedence.
128128
RemoveRegex []string `yaml:"remove_regex" json:"remove_regex"`
129-
// Path of commits to be excluded from parsing while calculating library changes.
130-
// If all files from commit belong to one of the paths it will be skipped.
129+
// A list of paths to exclude from the release.
130+
// Files matching these paths will not be considered part of a commit for this library.
131131
ReleaseExcludePaths []string `yaml:"release_exclude_paths,omitempty" json:"release_exclude_paths,omitempty"`
132132
// Specifying a tag format allows librarian to honor this format when creating
133133
// a tag for the release of the library. The replacement values of {id} and {version}
@@ -180,12 +180,12 @@ func (l *LibraryState) Validate() error {
180180
return fmt.Errorf("source_roots cannot be empty")
181181
}
182182
for i, p := range l.SourceRoots {
183-
if !isValidDirPath(p) {
183+
if !isValidRelativePath(p) {
184184
return fmt.Errorf("invalid source_path at index %d: %q", i, p)
185185
}
186186
}
187187
for i, p := range l.ReleaseExcludePaths {
188-
if !isValidDirPath(p) {
188+
if !isValidRelativePath(p) {
189189
return fmt.Errorf("invalid release_exclude_path at index %d: %q", i, p)
190190
}
191191
}
@@ -226,7 +226,7 @@ type API struct {
226226

227227
// Validate checks that the API is valid.
228228
func (a *API) Validate() error {
229-
if !isValidDirPath(a.Path) {
229+
if !isValidRelativePath(a.Path) {
230230
return fmt.Errorf("invalid path: %q", a.Path)
231231
}
232232
return nil
@@ -236,7 +236,7 @@ func (a *API) Validate() error {
236236
// plus path separators and the null byte.
237237
const invalidPathChars = "<>:\"|?*/\\\x00"
238238

239-
func isValidDirPath(pathString string) bool {
239+
func isValidRelativePath(pathString string) bool {
240240
if pathString == "" {
241241
return false
242242
}

‎internal/config/state_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ func TestIsValidDirPath(t *testing.T) {
397397
{"invalid null byte", "a/b\x00c", false},
398398
} {
399399
t.Run(test.name, func(t *testing.T) {
400-
if got := isValidDirPath(test.path); got != test.want {
400+
if got := isValidRelativePath(test.path); got != test.want {
401401
t.Errorf("isValidDirPath(%q) = %v, want %v", test.path, got, test.want)
402402
}
403403
})

‎internal/librarian/commit_version_analyzer.go‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package librarian
1717
import (
1818
"fmt"
1919
"log/slog"
20+
"path/filepath"
2021
"strings"
2122

2223
"github.com/googleapis/librarian/internal/config"
@@ -67,7 +68,7 @@ func convertToConventionalCommits(repo gitrepo.Repository, library *config.Libra
6768
if err != nil {
6869
return nil, fmt.Errorf("failed to get changed files for commit %s: %w", commit.Hash.String(), err)
6970
}
70-
if shouldExclude(files, library.ReleaseExcludePaths) {
71+
if !shouldInclude(files, library.SourceRoots, library.ReleaseExcludePaths) {
7172
continue
7273
}
7374
parsedCommits, err := conventionalcommits.ParseCommits(commit, library.ID)
@@ -85,22 +86,27 @@ func convertToConventionalCommits(repo gitrepo.Repository, library *config.Libra
8586
return conventionalCommits, nil
8687
}
8788

88-
// shouldExclude determines if a commit should be excluded from a release.
89-
// It returns true if all files in the commit match one of exclude paths.
90-
func shouldExclude(files, excludePaths []string) bool {
91-
for _, file := range files {
92-
excluded := false
93-
for _, excludePath := range excludePaths {
94-
if strings.HasPrefix(file, excludePath) {
95-
excluded = true
96-
break
97-
}
89+
// isUnderAnyPath returns true if the file is under any of the given paths.
90+
func isUnderAnyPath(file string, paths []string) bool {
91+
for _, p := range paths {
92+
rel, err := filepath.Rel(p, file)
93+
if err == nil && !strings.HasPrefix(rel, "..") {
94+
return true
9895
}
99-
if !excluded {
100-
return false
96+
}
97+
return false
98+
}
99+
100+
// shouldInclude determines if a commit should be included in a release.
101+
// It returns true if there is at least one file in the commit that is under a source_root
102+
// and not under a release_exclude_path.
103+
func shouldInclude(files, sourceRoots, excludePaths []string) bool {
104+
for _, file := range files {
105+
if isUnderAnyPath(file, sourceRoots) && !isUnderAnyPath(file, excludePaths) {
106+
return true
101107
}
102108
}
103-
return true
109+
return false
104110
}
105111

106112
// formatTag returns the git tag for a given library version.

‎internal/librarian/commit_version_analyzer_test.go‎

Lines changed: 85 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,49 +27,117 @@ import (
2727
"github.com/googleapis/librarian/internal/semver"
2828
)
2929

30-
func TestShouldExclude(t *testing.T) {
30+
func TestShouldInclude(t *testing.T) {
3131
t.Parallel()
3232
for _, test := range []struct {
3333
name string
3434
files []string
35+
sourceRoots []string
3536
excludePaths []string
3637
want bool
3738
}{
3839
{
39-
name: "no exclude paths",
40+
name: "file in source root, not excluded",
4041
files: []string{"a/b/c.go"},
42+
sourceRoots: []string{"a"},
4143
excludePaths: []string{},
42-
want: false,
44+
want: true,
4345
},
4446
{
45-
name: "file in exclude path",
47+
name: "file in source root, and excluded",
4648
files: []string{"a/b/c.go"},
49+
sourceRoots: []string{"a"},
4750
excludePaths: []string{"a/b"},
51+
},
52+
{
53+
name: "file not in source root",
54+
files: []string{"x/y/z.go"},
55+
sourceRoots: []string{"a"},
56+
excludePaths: []string{},
57+
},
58+
{
59+
name: "one file included, one file not in source root",
60+
files: []string{"a/b/c.go", "x/y/z.go"},
61+
sourceRoots: []string{"a"},
62+
excludePaths: []string{},
4863
want: true,
4964
},
5065
{
51-
name: "file not in exclude path",
52-
files: []string{"a/b/c.go"},
53-
excludePaths: []string{"d/e"},
54-
want: false,
66+
name: "one file included, one file excluded",
67+
files: []string{"a/b/c.go", "a/d/e.go"},
68+
sourceRoots: []string{"a"},
69+
excludePaths: []string{"a/d"},
70+
want: true,
5571
},
5672
{
57-
name: "one file in exclude path, one not",
58-
files: []string{"a/b/c.go", "d/e/f.go"},
73+
name: "all files excluded",
74+
files: []string{"a/b/c.go", "a/d/e.go"},
75+
sourceRoots: []string{"a"},
76+
excludePaths: []string{"a/b", "a/d"},
77+
},
78+
{
79+
name: "all files not in source root",
80+
files: []string{"x/y/c.go", "w/z/e.go"},
81+
sourceRoots: []string{"a"},
82+
excludePaths: []string{},
83+
},
84+
{
85+
name: "a file not in source root and a file in exclude path",
86+
files: []string{"a/b/c.go", "w/z/e.go"},
87+
sourceRoots: []string{"a"},
5988
excludePaths: []string{"a/b"},
60-
want: false,
6189
},
6290
{
63-
name: "all files in exclude paths",
64-
files: []string{"a/b/c.go", "d/e/f.go"},
65-
excludePaths: []string{"a/b", "d/e"},
91+
name: "a file in source root and not in exclude path, one file in exclude path and a file outside of source",
92+
files: []string{"a/d/c.go", "a/b/c.go", "w/z/e.go"},
93+
sourceRoots: []string{"a"},
94+
excludePaths: []string{"a/b"},
95+
want: true,
96+
},
97+
{
98+
name: "no source roots",
99+
files: []string{"a/b/c.go"},
100+
sourceRoots: []string{},
101+
excludePaths: []string{},
102+
},
103+
{
104+
name: "source root as prefix of another source root",
105+
files: []string{"aiplatform/file.go"},
106+
sourceRoots: []string{"ai"},
107+
excludePaths: []string{},
108+
},
109+
{
110+
name: "excluded path is a directory",
111+
files: []string{"foo/bar/baz.go"},
112+
sourceRoots: []string{"foo"},
113+
excludePaths: []string{"foo/bar"},
114+
},
115+
{
116+
name: "excluded path is a file, file matching it",
117+
files: []string{"foo/bar/go.mod"},
118+
sourceRoots: []string{"foo"},
119+
excludePaths: []string{"foo/bar/go.mod"},
120+
},
121+
{
122+
name: "excluded path is a file, file does not match it",
123+
files: []string{"foo/go.mod"},
124+
sourceRoots: []string{"foo"},
125+
excludePaths: []string{"foo/bar/go.mod"},
126+
want: true,
127+
},
128+
{
129+
name: "excluded path is a file with similar name",
130+
files: []string{"foo/bar/go.mod.bak"},
131+
sourceRoots: []string{"foo"},
132+
excludePaths: []string{"foo/bar/go.mod"},
66133
want: true,
67134
},
68135
} {
69136
t.Run(test.name, func(t *testing.T) {
70-
got := shouldExclude(test.files, test.excludePaths)
137+
t.Parallel()
138+
got := shouldInclude(test.files, test.sourceRoots, test.excludePaths)
71139
if got != test.want {
72-
t.Errorf("shouldExclude(%v, %v) = %v, want %v", test.files, test.excludePaths, got, test.want)
140+
t.Errorf("shouldInclude(%v, %v, %v) = %v, want %v", test.files, test.sourceRoots, test.excludePaths, got, test.want)
73141
}
74142
})
75143
}
@@ -207,7 +275,7 @@ func TestGetConventionalCommitsSinceLastRelease(t *testing.T) {
207275
},
208276
ChangedFilesInCommitValue: []string{"foo/a.txt"},
209277
},
210-
library: &config.LibraryState{ID: "foo"},
278+
library: &config.LibraryState{ID: "foo", SourceRoots: []string{"foo"}},
211279
wantErr: true,
212280
wantErrPhrase: "failed to parse commit",
213281
},
@@ -343,7 +411,6 @@ func TestNextVersion(t *testing.T) {
343411
},
344412
currentVersion: "1.0.0",
345413
wantVersion: "1.1.0",
346-
wantErr: false,
347414
},
348415
{
349416
name: "derive next returns error",
@@ -361,7 +428,6 @@ func TestNextVersion(t *testing.T) {
361428
},
362429
currentVersion: "1.2.3",
363430
wantVersion: "1.3.0",
364-
wantErr: false,
365431
},
366432
{
367433
name: "major change before nested commit results in major bump",
@@ -371,7 +437,6 @@ func TestNextVersion(t *testing.T) {
371437
},
372438
currentVersion: "1.2.3",
373439
wantVersion: "2.0.0",
374-
wantErr: false,
375440
},
376441
} {
377442
t.Run(test.name, func(t *testing.T) {

0 commit comments

Comments
 (0)