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.
| # The grep query tries to match `{artifact_id}:{released_version}:{current_version}`. | ||
| # The artifact_id must be exact otherwise multiple entries may match | ||
| version=$(cat "versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3) | ||
| version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3) |
There was a problem hiding this comment.
Prefer using standard CLI commands to retrieve tool versions rather than manually parsing configuration files, unless performance or latency issues are actively observed.
| version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3) | |
| version=$(librarian config get version "${artifact_id}") |
References
- Prefer using standard CLI commands (such as
librarian config get version) to retrieve tool versions rather than manually parsing configuration files, unless performance or latency issues are actively observed.
| mvn clean install -B -V -ntp -T 1C -DskipTests -Dclirr.skip -Denforcer.skip -Dmaven.javadoc.skip | ||
| repo_dir="${monorepoRoot}/${repo}" | ||
| if [ ! -d "${repo_dir}" ]; then | ||
| echo "Directory ${repo_dir} does not exist. Skipping or failed." |
There was a problem hiding this comment.
Error messages should be redirected to standard error (>&2) instead of standard output.
| echo "Directory ${repo_dir} does not exist. Skipping or failed." | |
| echo "Directory ${repo_dir} does not exist. Skipping or failed." >&2 |
| exit 1 | ||
| fi | ||
|
|
||
| pushd "${repo_dir}" |
There was a problem hiding this comment.
It is safer to append || exit 1 to pushd to ensure the script terminates if the directory change fails, preventing subsequent commands (like mvn clean install) from running in the wrong directory.
| pushd "${repo_dir}" | |
| pushd "${repo_dir}" || exit 1 |
| pushd "$repo" | ||
| repo_dir="${monorepoRoot}/${repo}" | ||
| if [ ! -d "${repo_dir}" ]; then | ||
| echo "Directory ${repo_dir} does not exist. Skipping or failed." |
There was a problem hiding this comment.
Error messages should be redirected to standard error (>&2) instead of standard output.
| echo "Directory ${repo_dir} does not exist. Skipping or failed." | |
| echo "Directory ${repo_dir} does not exist. Skipping or failed." >&2 |
| exit 1 | ||
| fi | ||
|
|
||
| pushd "${repo_dir}" |
There was a problem hiding this comment.
It is safer to append || exit 1 to pushd to ensure the script terminates if the directory change fails, preventing subsequent commands from running in the wrong directory.
| pushd "${repo_dir}" | |
| pushd "${repo_dir}" || exit 1 |
e8628bf to
9d06d44
Compare
9d06d44 to
dbc6eee
Compare
|
|
1 participant
WIP