feat(internal/gapicgen): fix genbot PR descriptions#13189
feat(internal/gapicgen): fix genbot PR descriptions#13189quartzmo merged 3 commits intogoogleapis:mainfrom
Conversation
When a `go-genproto` regeneration is triggered after a long period of inactivity, genbot would attempt to include all intervening commits in the pull request description. This could result a PR description being rejected by GitHub.. genbot now identifies only the commits that are relevant to the go-genproto regeneration by checking which commits affected the watched proto files. The new PR description now includes: - A list of only the relevant commits. - A list of the specific packages affected by each commit. fixes: googleapis#13188
Summary of ChangesHello @quartzmo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a long-standing issue where genbot would generate excessively verbose pull request descriptions for go-genproto regenerations, often leading to GitHub rejecting them due to length constraints. The core change introduces a more sophisticated commit filtering mechanism, ensuring that only commits directly impacting watched proto files are included in the PR description. Furthermore, the updated descriptions will now clearly enumerate the specific Go packages affected by each relevant commit, providing reviewers with a more focused and actionable summary of the changes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with overly large PR descriptions for go-genproto regenerations by intelligently filtering for relevant commits. The approach of checking which commits affect watched proto files is sound. The changes also enhance the PR description by listing the specific packages affected by each commit. The implementation is mostly solid, but I've provided one suggestion in internal/gapicgen/generator/generator.go to refactor the new logic. This refactoring improves efficiency by using a map for lookups, prevents potential duplicate package listings, and ensures a deterministic order for the affected packages, which improves the overall robustness and quality of the generated PR descriptions.
shollyman
left a comment
shollyman
left a comment
There was a problem hiding this comment.
I have silly questions about the new filtration, but LGTMing because I assume the error is in my understanding.
| return nil, err | ||
| } | ||
| changes, err := git.ParseChangeInfo(googleapisDir, commits) | ||
| affectedProtos := make(map[string][]string) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| } | ||
|
|
||
| // 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 provdied |
There was a problem hiding this comment.
nit: since you're already touching the line, maybe fix up the s/provdied/provided/ typo
The genbot would previously crash if it encountered a commit that deleted a proto file. This was because it was trying to read the file from the current filesystem, where it no longer existed. This change updates the genbot to read the file content from the historical commit directly, making the process resilient to file deletions and renames.i refs: googleapis#13188 refs: googleapis#13189
The genbot would previously crash if it encountered a commit that deleted a proto file. This was because it was trying to read the file from the current filesystem, where it no longer existed. This change updates the genbot to read the file content from the historical commit directly, making the process resilient to file deletions and renames.i refs: #13188 refs: #13189 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
3 participants
When a
go-genprotoregeneration is triggered after a long period of inactivity, genbot would attempt to include all intervening commits in the pull request description. This could result a PR description being rejected by GitHub.genbot now identifies only the commits that are relevant to the go-genproto regeneration by checking which commits affected the watched proto files.
The new PR description now includes:
fixes: #13188