Improve the PR builder to avoid multiple builds#26976
Open
madurangasiriwardena wants to merge 1 commit intowso2:masterfrom
Open
Improve the PR builder to avoid multiple builds#26976madurangasiriwardena wants to merge 1 commit intowso2:masterfrom
madurangasiriwardena wants to merge 1 commit intowso2:masterfrom
Conversation
Contributor
WalkthroughThe changes split the integrated build-and-test workflow into two separate phases: a centralized build phase that builds product-is without running tests, and a matrix-driven test phase that runs integration tests against the pre-built artifact. Complex test-activation logic is removed and Maven is configured to skip tests by default during the main build. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions<br/>(Workflow)
participant BS as Build Script<br/>(pr-builder-build.sh)
participant Artifacts as Artifact<br/>Storage
participant TS as Test Script<br/>(pr-builder-test.sh)
participant Runner as Test Runner<br/>(Matrix × 4)
rect rgba(100, 150, 200, 0.5)
Note over GHA,Artifacts: Build Phase (Sequential)
GHA->>BS: Execute build<br/>(clone, apply diff)
BS->>BS: Build product-is<br/>(mvn -Dmaven.test.skip=true)
BS->>Artifacts: Upload: product-is-build/,<br/>diff, Maven logs
end
rect rgba(200, 150, 100, 0.5)
Note over Artifacts,Runner: Test Phase (Parallel × 4 Runners)
Artifacts->>Runner: Download product-is-build
Artifacts->>Runner: Restore Maven cache
Runner->>TS: Execute test script<br/>(BUILDER_NUMBER, ENABLED_TESTS)
TS->>TS: Copy to per-runner dir<br/>(product-is-$BUILDER_NUMBER)
TS->>TS: Customize testng.xml<br/>(disable non-enabled tests)
TS->>TS: Run mvn install<br/>(integration module)
TS->>TS: Validate BUILD SUCCESS<br/>count vs. expected
TS->>Artifacts: Upload test results,<br/>logs, artifacts
Runner->>GHA: Report test results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/integration-test-runner.yml (2)
35-37: QuoteGITHUB_ENVto prevent word splitting.While
GITHUB_ENVtypically doesn't contain spaces, quoting is the safer practice and silences shellcheck warnings.Proposed fix
- run: echo "J21HOME=$JAVA_HOME" >> ${GITHUB_ENV} + run: echo "J21HOME=$JAVA_HOME" >> "${GITHUB_ENV}" - name: Build init - run: echo "CURRENT_MONTH=$(date +%Y-%m)" >> ${GITHUB_ENV} + run: echo "CURRENT_MONTH=$(date +%Y-%m)" >> "${GITHUB_ENV}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner.yml around lines 35 - 37, The workflow uses unquoted GITHUB_ENV in shell redirections which can cause word splitting; update the steps that append to GITHUB_ENV (the run commands that echo "J21HOME=$JAVA_HOME" and echo "CURRENT_MONTH=$(date +%Y-%m)") to quote the variable by redirecting into "${GITHUB_ENV}" instead of ${GITHUB_ENV} so the env file path is safely handled and shellcheck warnings are silenced.
116-118: QuoteGITHUB_ENVfor consistency.Same as the build job — quote the variable for safer shell scripting.
Proposed fix
- run: echo "J21HOME=$JAVA_HOME" >> ${GITHUB_ENV} + run: echo "J21HOME=$JAVA_HOME" >> "${GITHUB_ENV}" - name: Build init - run: echo "CURRENT_MONTH=$(date +%Y-%m)" >> ${GITHUB_ENV} + run: echo "CURRENT_MONTH=$(date +%Y-%m)" >> "${GITHUB_ENV}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test-runner.yml around lines 116 - 118, Update the echo redirection to quote the GITHUB_ENV variable for safer shell usage: in the step named "Build init" and the earlier JAVA_HOME export line, change the redirection target from >> ${GITHUB_ENV} to >> "${GITHUB_ENV}" so both echo commands that set JAVA_HOME (J21HOME/ JAVA_HOME) and CURRENT_MONTH write to the quoted "${GITHUB_ENV}"..github/scripts/pr-builder-test.sh (1)
62-67: Consider a more robust array membership check.The current pattern works for test names without spaces, but it's fragile and triggers shellcheck warnings. A loop-based approach is more explicit and robust.
Alternative using a helper function
+# Helper function to check if an element is in an array. +is_in_array() { + local needle="$1" + shift + for item in "$@"; do + [[ "$item" == "$needle" ]] && return 0 + done + return 1 +} + # Function to disable tests not in the enabled list. disable_tests() { local enabled_tests=$1 local testng_path="product-is-$BUILDER_NUMBER/modules/integration/tests-integration/tests-backend/src/test/resources/testng.xml" # Convert comma-separated string to array. IFS=',' read -ra ENABLED_ARRAY <<< "$enabled_tests" echo "Tests that will run:" printf '%s\n' "${ENABLED_ARRAY[@]}" echo -e "\nDisabling other tests:" for test in "${ALL_TESTS[@]}"; do - if [[ ! " ${ENABLED_ARRAY[@]} " =~ " ${test} " ]]; then + if ! is_in_array "$test" "${ENABLED_ARRAY[@]}"; then echo "- Disabling: $test" sed -i.bak "s/name=\"$test\"/& enabled=\"false\"/" "$testng_path" fi done }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/pr-builder-test.sh around lines 62 - 67, The membership check using [[ ! " ${ENABLED_ARRAY[@]} " =~ " ${test} " ]] is fragile and shellcheck‑warned; replace it with an explicit loop or helper to test equality against ENABLED_ARRAY. Modify the for test in "${ALL_TESTS[@]}" loop to call a small helper (e.g., contains_element) or inline a nested for loop that iterates over ENABLED_ARRAY and sets a flag if test == item, then only run the sed replacement (sed -i.bak "s/name=\"$test\"/& enabled=\"false\"/" "$testng_path") when the flag is false; reference variables ALL_TESTS, ENABLED_ARRAY, test, and testng_path when locating where to change. Ensure the helper or loop uses POSIX/portable string equality (=) rather than regex matching to avoid false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/pr-builder-build.sh:
- Line 322: The script currently runs the command "cd product-is-build" without
checking for errors; add explicit error handling so the script does not continue
in the wrong directory if the cd fails — update the location where "cd
product-is-build" is invoked to test its exit status and, on failure, print an
informative error and exit non-zero (e.g., mirror the earlier fix used for the
other cd invocation: check the command result and bail out with a message).
- Line 53: The script currently runs the command 'cd product-is-build' without
checking its result; modify the script so that after invoking 'cd
product-is-build' you verify the command succeeded and, if not, print a clear
error message and exit with a non-zero status (for example, check the exit code
and call exit 1 or use a short-circuit to log+exit); this ensures subsequent
steps do not run in the wrong directory if the directory change failed.
In @.github/scripts/pr-builder-test.sh:
- Line 107: The script currently runs the bare command "cd
product-is-$BUILDER_NUMBER" which can silently fail if the earlier directory
copy failed; update the script to verify the cd succeeded and abort on failure
(e.g., check the exit status of the cd and echo an error and exit non‑zero if it
fails, or use a conditional like "if ! cd product-is-$BUILDER_NUMBER; then ...;
fi") so that subsequent Maven commands do not run in the wrong directory.
- Around line 70-79: The substring check in get_expected_build_success_count
misfires for the "-with-restart" variant; replace the loose glob match on
enabled_tests with an exact-element check (e.g., split enabled_tests into an
array and compare each element to "is-test-adaptive-authentication-nashorn" or
use a bash regex with explicit boundaries like
(^|[[:space:]])is-test-adaptive-authentication-nashorn($|[[:space:]])) so only
the exact test name triggers the 17 return; update the conditional that
currently matches $enabled_tests to use this precise-match logic.
---
Nitpick comments:
In @.github/scripts/pr-builder-test.sh:
- Around line 62-67: The membership check using [[ ! " ${ENABLED_ARRAY[@]} " =~
" ${test} " ]] is fragile and shellcheck‑warned; replace it with an explicit
loop or helper to test equality against ENABLED_ARRAY. Modify the for test in
"${ALL_TESTS[@]}" loop to call a small helper (e.g., contains_element) or inline
a nested for loop that iterates over ENABLED_ARRAY and sets a flag if test ==
item, then only run the sed replacement (sed -i.bak "s/name=\"$test\"/&
enabled=\"false\"/" "$testng_path") when the flag is false; reference variables
ALL_TESTS, ENABLED_ARRAY, test, and testng_path when locating where to change.
Ensure the helper or loop uses POSIX/portable string equality (=) rather than
regex matching to avoid false positives.
In @.github/workflows/integration-test-runner.yml:
- Around line 35-37: The workflow uses unquoted GITHUB_ENV in shell redirections
which can cause word splitting; update the steps that append to GITHUB_ENV (the
run commands that echo "J21HOME=$JAVA_HOME" and echo "CURRENT_MONTH=$(date
+%Y-%m)") to quote the variable by redirecting into "${GITHUB_ENV}" instead of
${GITHUB_ENV} so the env file path is safely handled and shellcheck warnings are
silenced.
- Around line 116-118: Update the echo redirection to quote the GITHUB_ENV
variable for safer shell usage: in the step named "Build init" and the earlier
JAVA_HOME export line, change the redirection target from >> ${GITHUB_ENV} to >>
"${GITHUB_ENV}" so both echo commands that set JAVA_HOME (J21HOME/ JAVA_HOME)
and CURRENT_MONTH write to the quoted "${GITHUB_ENV}".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 462ab42c-38ff-4cae-8dc4-3b35d3624c69
📒 Files selected for processing (3)
.github/scripts/pr-builder-build.sh.github/scripts/pr-builder-test.sh.github/workflows/integration-test-runner.yml
| echo "PR is for the product-is itself. Applying diff and building without tests..." | ||
| echo "==========================================================" | ||
| cd product-is-$BUILDER_NUMBER | ||
| cd product-is-build |
Contributor
There was a problem hiding this comment.
Add error handling for cd command.
If cd product-is-build fails (e.g., clone failed silently), subsequent commands will execute in the wrong directory, potentially causing confusing failures or unintended side effects.
Proposed fix
- cd product-is-build
+ cd product-is-build || { echo "::error::Failed to cd into product-is-build"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd product-is-build | |
| cd product-is-build || { echo "::error::Failed to cd into product-is-build"; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 53-53: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/pr-builder-build.sh at line 53, The script currently runs
the command 'cd product-is-build' without checking its result; modify the script
so that after invoking 'cd product-is-build' you verify the command succeeded
and, if not, print a clear error message and exit with a non-zero status (for
example, check the exit code and call exit 1 or use a short-circuit to
log+exit); this ensures subsequent steps do not run in the wrong directory if
the directory change failed.
| fi | ||
|
|
||
| cd product-is-$BUILDER_NUMBER | ||
| cd product-is-build |
Contributor
There was a problem hiding this comment.
Add error handling for cd command.
Same issue as the earlier cd — if this fails, the script continues in the wrong directory.
Proposed fix
- cd product-is-build
+ cd product-is-build || { echo "::error::Failed to cd into product-is-build"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd product-is-build | |
| cd product-is-build || { echo "::error::Failed to cd into product-is-build"; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 322-322: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/pr-builder-build.sh at line 322, The script currently runs
the command "cd product-is-build" without checking for errors; add explicit
error handling so the script does not continue in the wrong directory if the cd
fails — update the location where "cd product-is-build" is invoked to test its
exit status and, on failure, print an informative error and exit non-zero (e.g.,
mirror the earlier fix used for the other cd invocation: check the command
result and bail out with a message).
Comment on lines
+70
to
+79
| # Function to get expected BUILD SUCCESS count based on enabled tests. | ||
| get_expected_build_success_count() { | ||
| local enabled_tests=$1 | ||
| # If is-test-adaptive-authentication-nashorn is enabled, expect 17 BUILD SUCCESS messages. | ||
| if [[ "$enabled_tests" == *"is-test-adaptive-authentication-nashorn"* ]]; then | ||
| echo "17" | ||
| else | ||
| echo "1" | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
Substring match incorrectly matches the -with-restart variant.
The condition *"is-test-adaptive-authentication-nashorn"* will match both:
is-test-adaptive-authentication-nashornis-test-adaptive-authentication-nashorn-with-restart
If only the -with-restart variant is enabled (not the base nashorn test), this function incorrectly returns 17 instead of 1.
Proposed fix using word boundary matching
get_expected_build_success_count() {
local enabled_tests=$1
- # If is-test-adaptive-authentication-nashorn is enabled, expect 17 BUILD SUCCESS messages.
- if [[ "$enabled_tests" == *"is-test-adaptive-authentication-nashorn"* ]]; then
+ # If is-test-adaptive-authentication-nashorn (but not -with-restart) is enabled,
+ # expect 17 BUILD SUCCESS messages due to the test's multi-module structure.
+ # Use comma boundaries to avoid matching the -with-restart variant.
+ if [[ ",$enabled_tests," == *",is-test-adaptive-authentication-nashorn,"* ]]; then
echo "17"
else
echo "1"
fi
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/pr-builder-test.sh around lines 70 - 79, The substring check
in get_expected_build_success_count misfires for the "-with-restart" variant;
replace the loose glob match on enabled_tests with an exact-element check (e.g.,
split enabled_tests into an array and compare each element to
"is-test-adaptive-authentication-nashorn" or use a bash regex with explicit
boundaries like
(^|[[:space:]])is-test-adaptive-authentication-nashorn($|[[:space:]])) so only
the exact test name triggers the 17 return; update the conditional that
currently matches $enabled_tests to use this precise-match logic.
| echo "" | ||
| echo "Running integration tests for runner $BUILDER_NUMBER..." | ||
| echo "==========================================================" | ||
| cd product-is-$BUILDER_NUMBER |
Contributor
There was a problem hiding this comment.
Add error handling for cd command.
If the directory copy on line 100 failed, this cd will fail and subsequent Maven commands will run in the wrong directory.
Proposed fix
-cd product-is-$BUILDER_NUMBER
+cd "product-is-$BUILDER_NUMBER" || { echo "::error::Failed to cd into product-is-$BUILDER_NUMBER"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd product-is-$BUILDER_NUMBER | |
| cd "product-is-$BUILDER_NUMBER" || { echo "::error::Failed to cd into product-is-$BUILDER_NUMBER"; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 107-107: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/pr-builder-test.sh at line 107, The script currently runs
the bare command "cd product-is-$BUILDER_NUMBER" which can silently fail if the
earlier directory copy failed; update the script to verify the cd succeeded and
abort on failure (e.g., check the exit status of the cd and echo an error and
exit non‑zero if it fails, or use a conditional like "if ! cd
product-is-$BUILDER_NUMBER; then ...; fi") so that subsequent Maven commands do
not run in the wrong directory.
Malith-19
reviewed
Mar 11, 2026
Contributor
There was a problem hiding this comment.
let's rename this file to indicate that we are building the target component + product is here
Malith-19
reviewed
Mar 11, 2026
| - name: Upload pre-built product-is tree | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: product-is-build |
Contributor
There was a problem hiding this comment.
when uploading the pack, since we are using common name, if multiple test runners running this will be overriden by the last upload. To avoid this we can add pr-number suffix and download using the same and delete the pack in the commenting step.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores