Skip to content

perf(ci): replace mvn help:evaluate with native bash and sed extraction - #14218

Merged
lqiu96 merged 1 commit into
googleapis:mainfrom
lqiu96:perf-replace-mvn-help-evaluate
Sep 1, 2026
Merged

perf(ci): replace mvn help:evaluate with native bash and sed extraction#14218
lqiu96 merged 1 commit into
googleapis:mainfrom
lqiu96:perf-replace-mvn-help-evaluate

Conversation

@lqiu96

@lqiu96 lqiu96 commented Aug 31, 2026

Copy link
Copy Markdown
Member

This PR is part 2 of 2 in a stacked series:

  1. ci: avoid running split ITs unexpectedly on unrelated changes (#12029) #14217: fix: avoid running split ITs unexpectedly on unrelated changes (Modified modules detection doesn't work correctly for modules with overlapping names #12029)
  2. This PR: perf(ci): replace mvn help:evaluate with native bash and sed extraction

Problem

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), evaluating project.version called maven-help-plugin:evaluate.

Changes

  • 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 directly 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 all temporary variables as local, used pure parameter expansion for whitespace trimming, and anchored paths.

@lqiu96
lqiu96 requested review from a team as code owners August 31, 2026 15:36
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 6521b89 to 0770ed3 Compare August 31, 2026 15:38

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

high

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)

medium

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:]]}"}"

@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from c66781c to 4a6a76b Compare August 31, 2026 15:53
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 0770ed3 to be49b9d Compare August 31, 2026 15:55
@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from 4a6a76b to a15b6a5 Compare August 31, 2026 16:02
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from be49b9d to c185259 Compare August 31, 2026 16:02
lqiu96 added a commit to lqiu96/google-cloud-java that referenced this pull request Aug 31, 2026
…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
@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from a15b6a5 to 54fb41a Compare August 31, 2026 16:20
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from c185259 to 5ccf52b Compare August 31, 2026 16:21
lqiu96 added a commit to lqiu96/google-cloud-java that referenced this pull request Aug 31, 2026
…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
@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from 54fb41a to 16b1841 Compare August 31, 2026 16:28
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 5ccf52b to d56af14 Compare August 31, 2026 16:29
lqiu96 added a commit to lqiu96/google-cloud-java that referenced this pull request Aug 31, 2026
…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
@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from 16b1841 to 6b8aa56 Compare August 31, 2026 19:50
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from d56af14 to 3340211 Compare August 31, 2026 19:51
lqiu96 added a commit to lqiu96/google-cloud-java that referenced this pull request Aug 31, 2026
…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
@lqiu96
lqiu96 force-pushed the fix-split-it-module-detection branch from 6b8aa56 to 86b6dfb Compare August 31, 2026 20:05
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 3340211 to 5151cab Compare August 31, 2026 20:05
@lqiu96
lqiu96 marked this pull request as draft August 31, 2026 21:17
lqiu96 added a commit that referenced this pull request Sep 1, 2026
#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`.
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 5151cab to 3f379b3 Compare September 1, 2026 02:08
@lqiu96
lqiu96 changed the base branch from fix-split-it-module-detection to main September 1, 2026 02:21
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 3f379b3 to 3d04da1 Compare September 1, 2026 02:30
@lqiu96
lqiu96 requested review from blakeli0 and whowes September 1, 2026 02:41
@lqiu96
lqiu96 marked this pull request as ready for review September 1, 2026 02:41
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 3d04da1 to 9f74cf2 Compare September 1, 2026 03:26
@lqiu96
lqiu96 requested review from a team as code owners September 1, 2026 03:26
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 9f74cf2 to bfff55a Compare September 1, 2026 03:46
@lqiu96
lqiu96 marked this pull request as draft September 1, 2026 04:19
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch 3 times, most recently from 568202b to 6bf8e1a Compare September 1, 2026 15:23
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.
@lqiu96
lqiu96 force-pushed the perf-replace-mvn-help-evaluate branch from 6bf8e1a to e4a8863 Compare September 1, 2026 15:32
@lqiu96
lqiu96 marked this pull request as ready for review September 1, 2026 15:38
@lqiu96
lqiu96 merged commit 744a522 into googleapis:main Sep 1, 2026
207 of 208 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants