Skip to content

feat(internal/gapicgen): fix genbot PR descriptions #13189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
quartzmo merged 3 commits into from
Oct 20, 2025
Merged

feat(internal/gapicgen): fix genbot PR descriptions #13189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f720935
feat(internal/gapicgen): fix genbot PR descriptions
quartzmo Oct 20, 2025
326e121
change affected packages to de-duped affected protos per Gemini review
quartzmo Oct 20, 2025
25ae052
fix provided typo
quartzmo Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion internal/gapicgen/generator/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"fmt"
"os"
"path/filepath"
"sort"
"strings"

"cloud.google.com/go/internal/gapicgen/git"
)
Expand Down Expand Up @@ -69,7 +71,43 @@ func gatherChanges(googleapisDir, genprotoDir string) ([]*git.ChangeInfo, error)
if err != nil {
return nil, err
}
changes, err := git.ParseChangeInfo(googleapisDir, commits)
affectedProtos := make(map[string][]string)
Copy link
Copy Markdown
Contributor

@shollyman shollyman Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question out the gate is whether we care about other artifacts like service config yaml files. e.g. if a service changes its retry policy, do we need to propagate that?

Copy link
Copy Markdown
Member Author

@quartzmo quartzmo Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the list of what's in scope for genbot currently. Its scope has been much reduced since the days when go-genproto contained all the GAPIC protos. I don't think there are any GAPIC-related files like service config yamls to worry about anymore: https://github.com/googleapis/google-cloud-go/blob/main/internal/gapicgen/generator/genproto.go#L56

var relevantCommits []string
for _, commit := range commits {
files, err := git.FilesChanged(googleapisDir, commit)
if err != nil {
return nil, err
}
protos := make(map[string]struct{})
for _, file := range files {
if !strings.HasSuffix(file, ".proto") {
continue
}
pkg, err := goPkg(filepath.Join(googleapisDir, file))
if err != nil {
return nil, err
}
var onWatchlist bool
for _, watchedPkg := range generateList {
if pkg == watchedPkg {
onWatchlist = true
break
}
}
if onWatchlist {
if _, ok := affectedProtos[commit]; !ok {
relevantCommits = append(relevantCommits, commit)
}
protos[file] = struct{}{}
}
}
for proto := range protos {
affectedProtos[commit] = append(affectedProtos[commit], proto)
}
sort.Strings(affectedProtos[commit])
}

changes, err := git.ParseChangeInfo(googleapisDir, relevantCommits, affectedProtos)
if err != nil {
return nil, err
}
Expand Down
19 changes: 14 additions & 5 deletions internal/gapicgen/git/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type ChangeInfo struct {
Body string
Title string
GoogleapisHash string
AffectedProtos []string
}

// FormatChanges turns a slice of changes into formatted string that will match
Expand Down Expand Up @@ -73,11 +74,18 @@ func truncateAndFormatChanges(changes []*ChangeInfo, onlyGapicChanges, truncate
if truncate {
startBody := strings.Index(body, "PiperOrigin-RevId")
if startBody != -1 {
body = fmt.Sprintf(" %s", body[startBody:])
body = " " + body[startBody:]
}
}

sb.WriteString(fmt.Sprintf("%s\n\n", body))
sb.WriteString(fmt.Sprintf("%s\n", body))
if len(c.AffectedProtos) > 0 {
sb.WriteString(" Affected protos:\n")
for _, proto := range c.AffectedProtos {
sb.WriteString(fmt.Sprintf(" - %s\n", proto))
}
}
sb.WriteString("\n")
}
// If the buffer is empty except for the "Changes:" text return an empty
// string.
Expand All @@ -88,7 +96,7 @@ func truncateAndFormatChanges(changes []*ChangeInfo, onlyGapicChanges, truncate
}

// ParseChangeInfo gets the ChangeInfo for a given googleapis hash.
func ParseChangeInfo(googleapisDir string, hashes []string) ([]*ChangeInfo, error) {
func ParseChangeInfo(googleapisDir string, hashes []string, affectedProtos map[string][]string) ([]*ChangeInfo, error) {
var changes []*ChangeInfo
for _, hash := range hashes {
// Get commit title and body
Expand All @@ -112,6 +120,7 @@ func ParseChangeInfo(googleapisDir string, hashes []string) ([]*ChangeInfo, erro
Title: title,
Body: body,
GoogleapisHash: hash,
AffectedProtos: affectedProtos[hash],
})
}
return changes, nil
Expand Down Expand Up @@ -182,9 +191,9 @@ func DeepClone(repo, dir string) error {
return err
}

// filesChanged returns a list of files changed in a commit for the provdied
// FilesChanged returns a list of files changed in a commit for the provided
// hash in the given gitDir.
func filesChanged(gitDir, hash string) ([]string, error) {
func FilesChanged(gitDir, hash string) ([]string, error) {
c := execv.Command("git", "show", "--pretty=format:", "--name-only", hash)
c.Dir = gitDir
b, err := c.Output()
Expand Down
6 changes: 6 additions & 0 deletions internal/gapicgen/git/git_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ func TestFormatChanges(t *testing.T) {
onlyGapics: true,
want: "",
},
{
name: "affected protos",
changes: []*ChangeInfo{{Title: "fix: foo", Body: "bar", AffectedProtos: []string{"foo.proto", "bar.proto"}}},
onlyGapics: false,
want: "\nChanges:\n\nfix: foo\n bar\n Affected protos:\n - foo.proto\n - bar.proto\n\n",
},
}

for _, tc := range tests {
Expand Down