USHIFT-6636: Change test-agent impl to align with greenboot-rs#6313
USHIFT-6636: Change test-agent impl to align with greenboot-rs#6313pmtk wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@pmtk: This pull request references USHIFT-6636 which is a valid jira issue. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test ? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved boot_counter logic from the test agent and migrated numeric action keys in-place; the CI ISO build script no longer attempts to download cached build data when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws-tests-arm e2e-aws-tests-bootc |
|
@pmtk: This pull request references USHIFT-6636 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/agent/microshift-test-agent.sh`:
- Around line 146-154: The jq walk() currently mutates numeric keys across the
whole AGENT_CFG, so update the transformation to target only the current
deployment object (the same selector you use earlier to extract actions) instead
of the entire file; pass the current deployment identifier into jq and apply the
decrement + remove-"0" logic only within that deployment's subtree (leaving
other deployments untouched) so only the booted deployment's numeric action keys
are shifted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 183c8d92-aaea-4955-ac62-ca9d3e583b4e
📒 Files selected for processing (3)
test/agent/microshift-test-agent.shtest/scenarios-bootc/presubmits/el98-src@upgrade-fails-then-recovers.shtest/scenarios/presubmits/el96-src@upgrade-fails-then-recovers.sh
pmtk
force-pushed
the
test-agent-greenboot-rs
branch
from
cba8d81 to
1618e22
Compare
|
/test e2e-aws-tests-arm e2e-aws-tests-bootc |
|
@pmtk: This pull request references USHIFT-6636 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-tests-arm e2e-aws-tests-bootc |
|
@pmtk: This pull request references USHIFT-6636 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bin/ci_phase_iso_build.sh`:
- Around line 191-198: Re-enable the cache-download conditional so
GOT_CACHED_DATA can become true when cache is available: restore the commented
block that checks HAS_CACHE_ACCESS and calls download_build_cache (setting
GOT_CACHED_DATA=true on success), ensure GOT_CACHED_DATA is initialized (e.g.,
GOT_CACHED_DATA=false) before use, and change the test from the brittle "if !
${GOT_CACHED_DATA}" to a proper shell test like "if [ \"$GOT_CACHED_DATA\" !=
true ]" so the warning on GOT_CACHED_DATA only prints when the download actually
failed; relevant symbols: GOT_CACHED_DATA, HAS_CACHE_ACCESS,
download_build_cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 991f7864-d627-4c1a-a39c-763451ce3d51
📒 Files selected for processing (1)
test/bin/ci_phase_iso_build.sh
| # if ${HAS_CACHE_ACCESS} ; then | ||
| # if download_build_cache ; then | ||
| # GOT_CACHED_DATA=true | ||
| # fi | ||
| # fi | ||
| if ! ${GOT_CACHED_DATA} ; then | ||
| echo "WARNING: Build cache is not available, rebuilding all the artifacts" | ||
| fi |
There was a problem hiding this comment.
Default build path now bypasses cache unconditionally.
At Line 191–195, the cache download logic is disabled, so GOT_CACHED_DATA never flips to true. That makes the Line 197 warning effectively unconditional and forces full rebuilds even when cache access exists.
Suggested fix
- # if ${HAS_CACHE_ACCESS} ; then
- # if download_build_cache ; then
- # GOT_CACHED_DATA=true
- # fi
- # fi
+ if ${HAS_CACHE_ACCESS} ; then
+ if download_build_cache ; then
+ GOT_CACHED_DATA=true
+ fi
+ fi📝 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.
| # if ${HAS_CACHE_ACCESS} ; then | |
| # if download_build_cache ; then | |
| # GOT_CACHED_DATA=true | |
| # fi | |
| # fi | |
| if ! ${GOT_CACHED_DATA} ; then | |
| echo "WARNING: Build cache is not available, rebuilding all the artifacts" | |
| fi | |
| if ${HAS_CACHE_ACCESS} ; then | |
| if download_build_cache ; then | |
| GOT_CACHED_DATA=true | |
| fi | |
| fi | |
| if ! ${GOT_CACHED_DATA} ; then | |
| echo "WARNING: Build cache is not available, rebuilding all the artifacts" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bin/ci_phase_iso_build.sh` around lines 191 - 198, Re-enable the
cache-download conditional so GOT_CACHED_DATA can become true when cache is
available: restore the commented block that checks HAS_CACHE_ACCESS and calls
download_build_cache (setting GOT_CACHED_DATA=true on success), ensure
GOT_CACHED_DATA is initialized (e.g., GOT_CACHED_DATA=false) before use, and
change the test from the brittle "if ! ${GOT_CACHED_DATA}" to a proper shell
test like "if [ \"$GOT_CACHED_DATA\" != true ]" so the warning on
GOT_CACHED_DATA only prints when the download actually failed; relevant symbols:
GOT_CACHED_DATA, HAS_CACHE_ACCESS, download_build_cache.
223f5b1 to
1989cfe
Compare
|
/test e2e-aws-tests-arm e2e-aws-tests-bootc |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/agent/microshift-test-agent.sh`:
- Around line 146-152: The jq transformation is decrementing the key "0" to "-1"
instead of removing it; update the pipeline around the with_entries/select steps
(the jq invocation used in the shell snippet) so that keys equal to "0" are
filtered out before any numeric decrement is applied, or restrict the decrement
to only numeric keys >= 1 (e.g., use a test like "^[1-9][0-9]*$"); adjust the
expression that uses with_entries and select so "0" never flows into the
decrement branch and is removed by the select(.key != "0") stage (refer to the
jq command in the diff that uses --arg key "${current_deployment_id}" and the
with_entries/select expressions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3942cd24-0c36-49d4-9b48-60cac3a2f0c6
📒 Files selected for processing (6)
test/agent/microshift-test-agent.shtest/bin/ci_phase_iso_build.shtest/scenarios/periodics/el96-prel@el98-src@upgrade-fails-and-rolls-back.shtest/scenarios/periodics/el98-src@upgrade-fails-cannot-backup.shtest/scenarios/presubmits/el98-src@downgrade-block.shtest/scenarios/presubmits/el98-src@upgrade-fails.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/bin/ci_phase_iso_build.sh
pmtk
force-pushed
the
test-agent-greenboot-rs
branch
from
1989cfe to
9b97758
Compare
|
@pmtk: This pull request references USHIFT-6636 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-tests-arm e2e-aws-tests-bootc |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pmtk
force-pushed
the
test-agent-greenboot-rs
branch
from
9b97758 to
bd9af65
Compare
|
/retest |
…narios due to greenboot behavior change" This reverts commit a54c1e1.
pmtk
force-pushed
the
test-agent-greenboot-rs
branch
from
bd9af65 to
2e8fb20
Compare
|
/retst |
|
/retest |
|
@pmtk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
3 participants
Summary by CodeRabbit
Refactor
Chores