chore: restore nightly protobuf compatibility check - #13884
Conversation
![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 refactors the nightly downstream Protobuf compatibility scripts to test local monorepo submodules directly instead of cloning them from GitHub, removing the special-casing for the google-cloud-java repository. Feedback on these changes recommends using standard CLI commands to retrieve tool versions rather than manually parsing versions.txt, redirecting error messages to standard error (>&2), and appending || exit 1 to pushd commands to ensure the scripts terminate safely if directory changes fail.
d8b7a8e to
24ef975
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Kokoro nightly downstream compatibility scripts to test local monorepo submodules instead of cloning external repositories. It also adds KMS dependencies to the installation list, excludes StorageITRunnerTest from storage tests, and configures test options for Java 8 compatibility. Feedback on the changes suggests improving Bash script efficiency and robustness, specifically by using native parameter expansion instead of cut, avoiding useless use of cat with grep, and using Bash arrays instead of unquoted string variables to prevent word splitting issues.
72be2a8 to
41ffb2b
Compare
41ffb2b to
b32cefc
Compare
whowes
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| - uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4 | ||
| with: | ||
| java-version: 11 | ||
| distribution: temurin |
There was a problem hiding this comment.
maybe we can add the cache: config here to see if this can resolve the deps (I don't think the deps will be changing too much).
There was a problem hiding this comment.
might be worth confirm with the Storage SMEs @ShreyasSinha if this is fine
There was a problem hiding this comment.
I dug into this more and it looks like the offending test just needed some GCP project name set to pass (not required to actually be a valid project), so I provided one and stopped excluding it here.
| - java-translate | ||
| # Default Protobuf-Java versions to use are specified here. Without this, the nightly workflow won't know | ||
| # which values to use and would resolve to ''. | ||
| protobuf-version: ${{ fromJSON(format('[{0}]', inputs.protobuf_runtime_versions || '"4.35.1"')) }} |
There was a problem hiding this comment.
qq, do we want to keep this aligned with the version of protobuf in pom-parent or with the latest protobuf-java available? CC: @blakeli0 if you have any thoughts.
I think the original intention was with the version of pom-parent, but that was when we were doing the migration. Now that 3.x and 4.x are mostly compatible we may not need that
There was a problem hiding this comment.
That's good context that this was originally intended to test against pom-parent's version. I switched to test against that version instead by default for now, but it might be worth discussing further whether we want to switch to testing against the latest version (or doing both, which is supported by passing in multiple versions via the inputs).
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
lgtm. added some small comments, but the changes generally looks good on my end and the manual workflow passes. thanks for the fix!
bc99190 to
695a897
Compare
695a897 to
5e40fdc
Compare
|
|
3 participants
Part of Issue #13867
Passing manual workflow run: https://github.com/googleapis/google-cloud-java/actions/runs/31056514739