perf(ci): replace mvn help:evaluate with native bash and sed extraction - #14218
Conversation
6521b89 to
0770ed3
Compare
There was a problem hiding this comment.
Code Review
This pull request optimizes the Kokoro CI build scripts by replacing slow Maven commands with pure Bash alternatives to extract module lists and showcase versions, significantly reducing build setup times. It also introduces a helper function to skip integration and GraalVM tests for unmodified modules, backed by new unit tests. The code review feedback points out two important issues in the common script: first, a potential 'dubious ownership' Git error in Docker environments because the safe directory configuration is bypassed in some jobs; and second, a risk of script termination under 'set -e' when using a here-string read to trim whitespace. Both issues should be addressed using the provided code suggestions.
I am having trouble creating individual review comments. Click here to see my feedback.
.kokoro/common.sh (350-354)
Since generate_modified_modules_list is no longer called in integration-single and graalvm-single jobs, is_module_modified will be executed without git config --global --add safe.directory having been run. In Kokoro/Docker environments where the build runs under a different user, this will cause git diff to fail with a 'dubious ownership' error.
We should ensure git config --global --add safe.directory "$(realpath .)" is run before executing git diff inside is_module_modified as well.
if [[ -z "${modified_files:-}" ]]; then
git config --global --add safe.directory "$(realpath .)"
local target_branch="${KOKORO_GITHUB_PULL_REQUEST_TARGET_BRANCH:-origin/main}"
local target_commit="${KOKORO_GITHUB_PULL_REQUEST_COMMIT:-HEAD}"
modified_files=$(git diff --name-only "${target_branch}...${target_commit}")
fi
.kokoro/common.sh (125-126)
Using read -r module <<< "${module}" for trimming whitespace can be risky if set -e is enabled in the calling script. If module is empty or contains only whitespace, read will return a non-zero exit status (1), causing the script to abort immediately.
Additionally, here-strings (<<<) can be less efficient as they may write to temporary files or spawn subshells depending on the Bash version. Reverting to the original pure parameter expansion is safer, faster, and avoids any exit status issues.
# Trim leading/trailing whitespace without spawning external processes
module="${module#"${module%%[![:space:]]*}"}"
module="${module%"${module##*[![:space:]]}"}"
c66781c to
4a6a76b
Compare
0770ed3 to
be49b9d
Compare
4a6a76b to
a15b6a5
Compare
be49b9d to
c185259
Compare
…eapis#12029) This PR is part 1 of 2 in a stacked series: 1. This PR (googleapis#14217): fix: avoid running split ITs unexpectedly on unrelated changes (googleapis#12029) 2. googleapis#14218: perf(ci): replace mvn help:evaluate with native bash and sed extraction ## Problem In split integration and GraalVM tests (e.g. integration-single and graalvm-single), module modification detection previously checked if the module name was present as a substring in modified_module_list. For modules with shared prefixes (such as java-bigquery vs java-bigquerystorage, java-bigquery-jdbc, or java-bigqueryconnection), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Modular Helper Functions with Single Responsibility**: - `populate_modified_files`: Encapsulates diff retrieval, caching in `modified_files`, and configuring git `safe.directory` for Docker container ownership mismatches. - `should_test_all_modules`: Purely checks global overrides that require testing all modules (parent POMs, core shared dependencies, and `TEST_ALL_MODULES="true"`). - `is_module_modified`: Strictly checks if files within a specific module directory were modified (`^${module}/`), avoiding prefix collisions. - **Explicit Job Control in Split Jobs**: - In `.kokoro/build.sh` (`integration-single` and `graalvm-single`), runs tests if either monorepo-wide testing is required or the specific module was modified: `! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"` -> skip. - **Updated generate_modified_modules_list**: - Reuses `populate_modified_files` and `should_test_all_modules`. - **Unit Tests**: - Added unit tests in `.kokoro/common_test.sh` for `is_module_modified` and `should_test_all_modules`. Fixes googleapis#12029
a15b6a5 to
54fb41a
Compare
c185259 to
5ccf52b
Compare
…eapis#12029) This PR is part 1 of 2 in a stacked series: 1. This PR (googleapis#14217): fix: avoid running split ITs unexpectedly on unrelated changes (googleapis#12029) 2. googleapis#14218: perf(ci): replace mvn help:evaluate with native bash and sed extraction ## Problem In split integration and GraalVM tests (e.g. integration-single and graalvm-single), module modification detection previously checked if the module name was present as a substring in modified_module_list. For modules with shared prefixes (such as java-bigquery vs java-bigquerystorage, java-bigquery-jdbc, or java-bigqueryconnection), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Modular Helper Functions with Single Responsibility**: - `populate_modified_files`: Encapsulates diff retrieval, caching in `modified_files`, and configuring git `safe.directory` for Docker container ownership mismatches. - `should_test_all_modules`: Purely checks global overrides that require testing all modules (parent POMs, core shared dependencies, and `TEST_ALL_MODULES="true"`). - `is_module_modified`: Strictly checks if files within a specific module directory were modified (`^${module}/`), avoiding prefix collisions. - **Explicit Job Control in Split Jobs**: - In `.kokoro/build.sh` (`integration-single` and `graalvm-single`), runs tests if either monorepo-wide testing is required or the specific module was modified: `! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"` -> skip. - **Updated generate_modified_modules_list**: - Reuses `populate_modified_files` and `should_test_all_modules`. - **Unit Tests**: - Added unit tests in `.kokoro/common_test.sh` for `is_module_modified` and `should_test_all_modules`. Fixes googleapis#12029
54fb41a to
16b1841
Compare
5ccf52b to
d56af14
Compare
…eapis#12029) This PR is part 1 of 2 in a stacked series: 1. This PR (googleapis#14217): fix: avoid running split ITs unexpectedly on unrelated changes (googleapis#12029) 2. googleapis#14218: perf(ci): replace mvn help:evaluate with native bash and sed extraction ## Problem In split integration and GraalVM tests (e.g. integration-single and graalvm-single), module modification detection previously checked if the module name was present as a substring in modified_module_list. For modules with shared prefixes (such as java-bigquery vs java-bigquerystorage, java-bigquery-jdbc, or java-bigqueryconnection), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Modular Helper Functions with Single Responsibility**: - `populate_modified_files`: Encapsulates diff retrieval, caching in `modified_files`, and configuring git `safe.directory` for Docker container ownership mismatches. - `should_test_all_modules`: Purely checks global overrides that require testing all modules (parent POMs, core shared dependencies, and `TEST_ALL_MODULES="true"`). - `is_module_modified`: Strictly checks if files within a specific module directory were modified (`^${module}/`), avoiding prefix collisions. - **Explicit Job Control in Split Jobs**: - In `.kokoro/build.sh` (`integration-single` and `graalvm-single`), runs tests if either monorepo-wide testing is required or the specific module was modified: `! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"` -> skip. - **Updated generate_modified_modules_list**: - Reuses `populate_modified_files` and `should_test_all_modules`. - **Unit Tests**: - Added unit tests in `.kokoro/common_test.sh` for `is_module_modified` and `should_test_all_modules`. Fixes googleapis#12029
16b1841 to
6b8aa56
Compare
d56af14 to
3340211
Compare
…eapis#12029) This PR is part 1 of 2 in a stacked series: 1. This PR (googleapis#14217): fix: avoid running split ITs unexpectedly on unrelated changes (googleapis#12029) 2. googleapis#14218: perf(ci): replace mvn help:evaluate with native bash and sed extraction ## Problem In split integration and GraalVM tests (e.g. integration-single and graalvm-single), module modification detection previously checked if the module name was present as a substring in modified_module_list. For modules with shared prefixes (such as java-bigquery vs java-bigquerystorage, java-bigquery-jdbc, or java-bigqueryconnection), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Modular Helper Functions with Single Responsibility**: - `populate_modified_files`: Encapsulates diff retrieval, caching in `modified_files`, and configuring git `safe.directory` for Docker container ownership mismatches. - `should_test_all_modules`: Purely checks global overrides that require testing all modules (parent POMs, core shared dependencies, and `TEST_ALL_MODULES="true"`). - `is_module_modified`: Strictly checks if files within a specific module directory were modified (`^${module}/`), avoiding prefix collisions. - **Explicit Job Control in Split Jobs**: - In `.kokoro/build.sh` (`integration-single` and `graalvm-single`), runs tests if either monorepo-wide testing is required or the specific module was modified: `! should_test_all_modules && ! is_module_modified "${BUILD_SUBDIR}"` -> skip. - **Updated generate_modified_modules_list**: - Reuses `populate_modified_files` and `should_test_all_modules`. - **Unit Tests**: - Added unit tests in `.kokoro/common_test.sh` for `is_module_modified` and `should_test_all_modules`. Fixes googleapis#12029
6b8aa56 to
86b6dfb
Compare
3340211 to
5151cab
Compare
#14217) Fixes #12029 (b/487770623) This PR is part 1 of 2 in a stacked series: 1. This PR (#14217): `fix: avoid running split ITs unexpectedly on unrelated changes (#12029)` 2. #14218: `perf(ci): replace mvn help:evaluate with native bash and sed extraction` --- ## Problem In split integration and GraalVM tests (e.g. `integration-single` and `graalvm-single`), module modification detection previously checked if the module name was present as a substring in `modified_module_list`. For modules with shared prefixes (such as `java-bigquery` vs `java-bigquerystorage`, `java-bigquery-jdbc`, or `java-bigqueryconnection`), modifying one module could trigger tests for other prefix-sharing modules. ## Changes - **Module Boundary Anchoring (`is_module_modified`)**: Added `is_module_modified` in `.kokoro/common.sh` using exact directory prefix matching (`^${module}/`). - **Global Overrides**: `is_module_modified` respects parent POM modifications (`google-cloud-(pom|jar)-parent/pom.xml`), shared dependency modifications (`sdk-platform-java/java-shared-dependencies`), and `TEST_ALL_MODULES="true"` so that dependency and parent updates properly verify downstream integration suites. - **Short-Circuit in Split Jobs**: Updated `integration-single` and `graalvm-single` in `.kokoro/build.sh` to check `is_module_modified "${BUILD_SUBDIR}"` directly. - **Updated Shared Dependencies Path**: Updated `shared_dependencies_modified` in `.kokoro/common.sh` to match `sdk-platform-java/java-shared-dependencies`. - **Unit Tests**: Added unit test coverage in `.kokoro/common_test.sh` for `is_module_modified` covering empty inputs, prefix collision prevention, parent pom modification, shared dependencies modification, and `TEST_ALL_MODULES`.
5151cab to
3f379b3
Compare
3f379b3 to
3d04da1
Compare
3d04da1 to
9f74cf2
Compare
9f74cf2 to
bfff55a
Compare
568202b to
6bf8e1a
Compare
In Kokoro CI and automation scripts, heavy Maven JVM invocations were used to parse POM values: - In generate_modified_modules_list (.kokoro/common.sh), evaluating project.modules launched a full JVM and evaluated the monorepo POMs, taking 20–30+ seconds on every single CI run. - In downstream-build.sh (.kokoro/presubmit/downstream-build.sh) and showcase-native.sh (sdk-platform-java/.kokoro/presubmit/showcase-native.sh), evaluating gapic-showcase.version launched Maven JVM processes. - In update_javadoc.sh (google-auth-library-java/scripts/update_javadoc.sh), maven-help-plugin:evaluate was called to obtain the project version. Optimizations: - Pure-Bash Module Extraction: Updated generate_modified_modules_list to use extract_pom_modules pom.xml, extracting all 270 modules in ~0.02s without JVM boot overhead. - Sed Showcase Version Extraction: Updated downstream-build.sh and showcase-native.sh to parse <gapic-showcase.version> directly using sed with fail-fast validation, and added --fail to curl. - Sed Javadoc Version Extraction: Updated update_javadoc.sh to parse project version from pom.xml using sed, skipping parent POM declarations. - Unit Tests: Added automated unit tests in .kokoro/common_test.sh for extract_pom_modules and generate_modified_modules_list. - Refactoring & Best Practices: Scoped temporary variables as local, used pure-bash whitespace trimming and string manipulations, and anchored paths.
6bf8e1a to
e4a8863
Compare
This PR is part 2 of 2 in a stacked series:
Problem
In Kokoro CI and automation scripts, heavy Maven JVM invocations were used to parse POM values:
generate_modified_modules_list(.kokoro/common.sh), evaluatingproject.moduleslaunched a full JVM and evaluated the monorepo POMs, taking 20–30+ seconds on every single CI run.downstream-build.sh(.kokoro/presubmit/downstream-build.sh) andshowcase-native.sh(sdk-platform-java/.kokoro/presubmit/showcase-native.sh), evaluatinggapic-showcase.versionlaunched Maven JVM processes.update_javadoc.sh(google-auth-library-java/scripts/update_javadoc.sh), evaluatingproject.versioncalledmaven-help-plugin:evaluate.Changes
generate_modified_modules_listto useextract_pom_modules pom.xml, extracting all 270 modules in ~0.02s without JVM boot overhead.downstream-build.shandshowcase-native.shto parse<gapic-showcase.version>directly usingsedwith fail-fast validation, and added--failto curl.update_javadoc.shto parse project version directly usingsed, skipping parent POM declarations..kokoro/common_test.shforextract_pom_modulesandgenerate_modified_modules_list.local, used pure parameter expansion for whitespace trimming, and anchored paths.