test: generate libraries with protobuf 4.x #4257
Conversation
product-auto-label
bot
added
size: xl
Summary of ChangesHello @diegomarquezp, 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 significantly upgrades the project's underlying infrastructure and expands its API surface. It ensures compatibility with Protobuf 4.x by updating reflection configurations, introduces robust tooling for publishing modules to the Bazel Central Registry, and integrates new CI/CD workflows for automated API updates and index generation. Additionally, it brings in new API definitions for Google Actions and Ad Manager, reflecting an expansion of supported services. Highlights
Ignored Files
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 appears to be part of an effort to generate libraries using Protobuf 4.x. The changes include updating GraalVM reflection configurations for new Protobuf classes and adding a very large number of new files, including API definitions, build configurations, and helper scripts under an output/ directory.
My review focuses on the new scripts and configuration files, as the protobuf and build files seem to be auto-generated. I've identified a few areas for improvement in the shell scripts for robustness and correctness, such as using more stable methods for parsing command output, fixing a typo, removing unused code, and updating a default configuration value for consistency. I also have a suggestion for the GitHub Actions workflow to improve its reproducibility.
|
|
||
| readonly DEFAULT_REPO="googleapis" | ||
| readonly DEFAULT_ORG="googleapis" | ||
| readonly DEFAULT_PROTOBUF_VERSION="21.7" |
There was a problem hiding this comment.
The default protobuf version is set to 21.7, which is from the protobuf 3.x series. However, the WORKSPACE file in this repository uses version 25.7, and the PR title suggests a move to protobuf 4.x. For consistency and to align with the repository's dependencies, consider updating the default version here. This would also apply to the README.md file which documents this default.
| readonly DEFAULT_PROTOBUF_VERSION="21.7" | |
| readonly DEFAULT_PROTOBUF_VERSION="25.7" |
| export pr_creation_output=$(bash -c "${pr_command}") | ||
| pr_url=$(grep '/pull/' <<< "${pr_creation_output}") |
There was a problem hiding this comment.
Parsing the human-readable output of gh pr create with grep can be fragile and might break if the output format of the gh CLI changes in the future. To make this more robust, you can use the --json flag to get structured output and parse it with the -q flag to extract the URL directly.
| export pr_creation_output=$(bash -c "${pr_command}") | |
| pr_url=$(grep '/pull/' <<< "${pr_creation_output}") | |
| pr_url=$(bash -c "${pr_command} --json url -q .url") |
| gh pr comment --body "${SKIP_FLAG_CHECK_COMMENT}" "${pr_url}" | ||
| echo "Done! You can see the created PR in ${pr_url}" | ||
| else | ||
| echo "The branch is ready. You can create a PR by runing:" |
There was a problem hiding this comment.
There is a typo in the log message. runing should be running.
| echo "The branch is ready. You can create a PR by runing:" | |
| echo "The branch is ready. You can create a PR by running:" |
| -d|--dry_run) | ||
| dry_run="$2" | ||
| shift | ||
| ;; |
There was a problem hiding this comment.
The --dry_run option is parsed here, but the dry_run variable is never used within the script. This appears to be either dead code or an unimplemented feature. To avoid confusion, it should be removed or the feature should be implemented.
| set -e | ||
|
|
||
| SHARED=$(mktemp -d) | ||
| BRANCH="update-binary-`date +%Y%m%dT%H%M%S`" |
There was a problem hiding this comment.
The use of backticks (`) for command substitution is generally discouraged in modern shell scripting. The $(...) syntax is preferred as it is more readable and can be nested safely.
| BRANCH="update-binary-`date +%Y%m%dT%H%M%S`" | |
| BRANCH="update-binary-$(date +%Y%m%dT%H%M%S)" |
| apt-get install -y jq | ||
| - name: Download discovery docs | ||
| run: | | ||
| curl https://raw.githubusercontent.com/googleapis/discovery-artifact-manager/refs/heads/master/discoveries/compute.${{ matrix.version }}.json --output /tmp/compute.${{ matrix.version }}.json |
There was a problem hiding this comment.
Fetching the discovery document from the master branch of another repository can make this workflow less reproducible and potentially flaky. If the file is moved or the branch is renamed, this step will fail. Consider using a specific commit SHA or a tag instead of a branch name to ensure you are fetching a known, stable version of the file.
1 participant
No description provided.