USHIFT-6073: Add "from previous Z" upgrade path on releases scenarios#5862
USHIFT-6073: Add "from previous Z" upgrade path on releases scenarios#5862agullon wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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
|
/retest |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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. |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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. |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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. |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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-bootc-release-arm e2e-aws-tests-bootc-release |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
/hold I would like us to try implementing the same without adding extra options / dimensions to the |
openshift-ci
bot
added
the
do-not-merge/hold
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
@ggiguash can you please review this PR again please? I modified the implementation with your suggestion. See PR description. Important, CI jobs are using custom S3 bucket to not mess up the cache: |
|
/test e2e-aws-tests-bootc-release e2e-aws-tests-release e2e-aws-tests-bootc-release-arm e2e-aws-tests-release-arm |
|
/test e2e-aws-tests-bootc-release |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aws-tests-cache |
|
/test e2e-aws-tests-bootc-release |
|
/retest |
|
/test e2e-aws-tests-periodic-arm |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
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:
WalkthroughReplaces static BREW_Y0/Y1/Y2 with discovery-based PREV_ZSTREAM/YMINUS1/YMINUS2 variables and adjusts LREL selection; updates Brew RPM download flow (new find_package/brew_cli_download), image extraction vars, RPM lists/exports, templates, adds prev-zstream containerfile and two bootc test scenarios, and hardcodes the AWS build-cache bucket name. Changes
Sequence Diagram(s)sequenceDiagram
actor Operator
participant Manager as "manage_brew_rpms.sh"
participant Finder as "find_package()"
participant Brew as "brew CLI/API"
participant FS as "Filesystem"
Operator->>Manager: action_download(ver, main_dir, ver_type)
Manager->>Finder: compute candidate package names (latest, prev-zstream, Y-1, Y-2)
Finder-->>Manager: return package list
loop per package candidate
Manager->>Brew: brew download <package>
alt success
Brew-->>FS: write package to main_dir/sub_dir/arch
FS-->>Manager: confirm file
else fail
Manager->>Operator: echo warning (or fail on final candidate)
Manager->>Brew: attempt curl fallback (if configured)
end
end
Manager-->>Operator: report success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/bin/build_images.sh (1)
724-735:⚠️ Potential issue | 🟠 MajorAdd BREW_PREV_ZSTREAM_VERSION to container image extraction.
BREW_PREV_ZSTREAM_VERSION is defined and actively used in prev-zstream containerfiles but is missing from the container image extraction block. All other BREW_* versions follow a consistent pattern with guard checks; PREV_ZSTREAM should be included to ensure its release-info images are captured during mirror setup.
Required fix
[ -n "${BREW_LATEST_ZSTREAM_VERSION}" ] && extract_container_images "${BREW_LATEST_ZSTREAM_VERSION}" "${BREW_REPO}" "${CONTAINER_LIST}" + [ -n "${BREW_PREV_ZSTREAM_VERSION}" ] && extract_container_images "${BREW_PREV_ZSTREAM_VERSION}" "${BREW_REPO}" "${CONTAINER_LIST}" [ -n "${BREW_YMINUS1_VERSION}" ] && extract_container_images "${BREW_YMINUS1_VERSION}" "${BREW_REPO}" "${CONTAINER_LIST}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/build_images.sh` around lines 724 - 735, The container extraction block in build_images.sh misses BREW_PREV_ZSTREAM_VERSION; add a guarded call matching the other BREW_* lines so its images are extracted. Specifically, add a line like the existing guarded lines using BREW_PREV_ZSTREAM_VERSION and BREW_REPO with extract_container_images (e.g., [ -n "${BREW_PREV_ZSTREAM_VERSION}" ] && extract_container_images "${BREW_PREV_ZSTREAM_VERSION}" "${BREW_REPO}" "${CONTAINER_LIST}"), placed with the other BREW_* checks so prev-zstream release-info images are included.test/bin/pyutils/build_bootc_images.py (1)
112-126:⚠️ Potential issue | 🟠 MajorMissing
BREW_PREV_ZSTREAM_VERSIONinitialization.The new containerfile (
rhel96-bootc-brew-prev-zstream-with-optional.containerfile) usesBREW_PREV_ZSTREAM_VERSIONfor templating, but this variable is not declared or initialized in this file. OnlyBREW_LATEST_ZSTREAM_VERSION,BREW_YMINUS1_VERSION, andBREW_YMINUS2_VERSIONare handled.Add the missing variable:
🐛 Proposed fix to add BREW_PREV_ZSTREAM_VERSION
global BREW_LATEST_ZSTREAM_VERSION + global BREW_PREV_ZSTREAM_VERSION global BREW_YMINUS1_VERSION global BREW_YMINUS2_VERSIONBREW_LATEST_ZSTREAM_VERSION = common.get_env_var('BREW_LATEST_ZSTREAM_VERSION') + BREW_PREV_ZSTREAM_VERSION = common.get_env_var('BREW_PREV_ZSTREAM_VERSION') BREW_YMINUS1_VERSION = common.get_env_var('BREW_YMINUS1_VERSION') BREW_YMINUS2_VERSION = common.get_env_var('BREW_YMINUS2_VERSION')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/build_bootc_images.py` around lines 112 - 126, The snippet fails to declare/initialize BREW_PREV_ZSTREAM_VERSION which is required by rhel96-bootc-brew-prev-zstream-with-optional.containerfile; add the missing global and initialize it from the environment using common.get_env_var just like the other vars (i.e., add a global BREW_PREV_ZSTREAM_VERSION and a line BREW_PREV_ZSTREAM_VERSION = common.get_env_var('BREW_PREV_ZSTREAM_VERSION') alongside the existing BREW_LATEST_ZSTREAM_VERSION / BREW_YMINUS1_VERSION / BREW_YMINUS2_VERSION assignments).
♻️ Duplicate comments (1)
test/bin/manage_build_cache.sh (1)
10-11:⚠️ Potential issue | 🔴 CriticalRemove hardcoded test bucket before merging.
Line 11 unconditionally overwrites the bucket name configured on line 10, disabling both the default value and any environment-provided override. This will redirect all cache operations to the test bucket if merged.
AWS_BUCKET_NAME="${AWS_BUCKET_NAME:-microshift-build-cache}" -AWS_BUCKET_NAME="microshift-build-ushift-6073-cache-us-west-2",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_build_cache.sh` around lines 10 - 11, The script currently hardcodes AWS_BUCKET_NAME on the second line which overrides the default and any environment-provided value; remove the hardcoded assignment (the line setting AWS_BUCKET_NAME="microshift-build-ushift-6073-cache-us-west-2") so that only the parameterized assignment AWS_BUCKET_NAME="${AWS_BUCKET_NAME:-microshift-build-cache}" remains, or if a test bucket is needed keep it behind an explicit conditional or CI-only guard that sets AWS_BUCKET_NAME only when a TEST_ENV variable is present.
🧹 Nitpick comments (4)
test/bin/manage_brew_rpms.sh (4)
52-57: Variablesver_xandver_yare not declaredlocal, causing scope leakage.These variables will persist in the calling scope after the function returns, which can lead to unexpected behavior if
find_packageis called multiple times with different versions.Proposed fix
# Extract version components - ver_x=$(echo "${ver}" | cut -d'.' -f1) - ver_y=$(echo "${ver}" | cut -d'.' -f2) + local ver_x ver_y + ver_x=$(echo "${ver}" | cut -d'.' -f1) + ver_y=$(echo "${ver}" | cut -d'.' -f2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 52 - 57, Inside the find_package function, ver_x and ver_y are not declared local and leak to the caller; make them local by adding local declarations for ver_x and ver_y (and any other temporary vars used for version math, e.g., ver_prev_y if present) before assigning via cut and arithmetic so the variables don't persist after the function returns.
176-181: Consider handling unexpectedver_typevalues infinal_sub_dirassignment.If
ver_typeis neitherec,rc,nightly, norzstream,final_sub_dirremains unset, which could cause unexpected directory paths. Whileaction_downloadvalidatesver_typebefore calling this function,brew_cli_downloadis public and could be called directly.Proposed defensive fix
# Format sub_dir for EC, RC and nightly + local final_sub_dir if [ "${ver_type}" = "ec" ] || [ "${ver_type}" = "rc" ] || [ "${ver_type}" = "nightly" ] ; then final_sub_dir=$(echo "${sub_dir}" | sed -E 's/(.*)(~)(.*)(rc|ec|nightly)(.*)/\1-\4/g') elif [ "${ver_type}" = "zstream" ] ; then final_sub_dir="${sub_dir}" + else + echo "ERROR: Invalid version_type '${ver_type}' in brew_cli_download" + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 176 - 181, final_sub_dir may be left unset when ver_type is not ec/rc/nightly/zstream; update the logic in the function that sets final_sub_dir (used by brew_cli_download and referenced from action_download) to handle unexpected ver_type values by either assigning a sensible default (e.g., final_sub_dir="${sub_dir}" or a normalized fallback) or explicitly failing with an error message; ensure the code path covers the existing if/elif block and adds an else branch that sets final_sub_dir or exits, so brew_cli_download never returns with final_sub_dir unset.
102-108: Variablespackage_latest,package_prev,package_yminus1,package_yminus2are not declaredlocal.Similar to the earlier issue, these variables leak to the outer scope.
Proposed fix
case ${ver_type} in zstream) + local package_latest package_prev package_yminus1 package_yminus2 package_latest="$(find_package "${ver}" "${ver_type}" "0" "0")" package_prev="$(find_package "${ver}" "${ver_type}" "0" "1")" package_yminus1="$(find_package "${ver}" "${ver_type}" "1" "0")" package_yminus2="$(find_package "${ver}" "${ver_type}" "2" "0")" package_list=("${package_latest}" "${package_yminus1}" "${package_yminus2}" "${package_prev}") ;; nightly|rc|ec) + local package_latest package_latest="$(find_package "${ver}" "${ver_type}")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 102 - 108, The variables package_latest, package_prev, package_yminus1, and package_yminus2 in the zstream case of the case block (where find_package is invoked) are not declared local and thus leak to outer scope; fix by declaring them local before assignment (i.e., make package_latest, package_prev, package_yminus1, package_yminus2 local in the zstream branch) so they are scoped to the function/block that contains this case; also ensure package_list remains correctly assigned after those local declarations.
176-181: Variablefinal_sub_diris not declaredlocal.This continues the pattern of scope leakage for function-internal variables.
Proposed fix
# Format sub_dir for EC, RC and nightly + local final_sub_dir if [ "${ver_type}" = "ec" ] || [ "${ver_type}" = "rc" ] || [ "${ver_type}" = "nightly" ] ; then final_sub_dir=$(echo "${sub_dir}" | sed -E 's/(.*)(~)(.*)(rc|ec|nightly)(.*)/\1-\4/g') elif [ "${ver_type}" = "zstream" ] ; then final_sub_dir="${sub_dir}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 176 - 181, The variable final_sub_dir is being assigned inside the conditional but not declared local, causing scope leakage; inside the function that contains this block (the function that uses ver_type and sub_dir), add a local declaration for final_sub_dir before its first assignment and then use final_sub_dir in the existing conditional that formats sub_dir for EC/RC/nightly (the block referencing ver_type, sub_dir and the sed transform) so the variable is function-scoped and does not leak to the global environment.
🤖 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/assets/common_versions.sh.template`:
- Around line 152-153: The current logic assigns LATEST_ZSTREAM_VERSION and
PREV_ZSTREAM_VERSION from the same find/sort pipeline which yields identical
values when only one z-stream exists; change the approach to first capture the
sorted list of matches (from BREW_RPM_SOURCE for pattern using MINOR_VERSION)
into a variable/array, set LATEST_ZSTREAM_VERSION to the last element and set
PREV_ZSTREAM_VERSION to the second-to-last element or to an empty value if the
list length is 1 (so PREV is not identical to LATEST), and ensure downstream
code can handle an empty PREV_ZSTREAM_VERSION.
In `@test/bin/build_rpms.sh`:
- Around line 124-129: Add an inline comment next to the failing Y-2 zstream
download invocation (the bash call that uses "${YMINUS2_MINOR_VERSION}" and ends
with "&& return 1") explaining why this download must fail the function while
the other manage_brew_rpms.sh download calls for RC, EC, nightly and Y-1 only
emit warnings; reference that this is intentional because the Y-2 zstream RPM is
required for upgrade testing (or whatever the policy is) so failure should
abort, and briefly state that other artifacts are optional so they only warn.
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 633-638: The code currently calls extract_container_images for
BREW_LATEST_ZSTREAM_VERSION, BREW_YMINUS1_VERSION, and BREW_YMINUS2_VERSION but
omits BREW_PREV_ZSTREAM_VERSION, causing missing images for the prev-zstream
upgrade; add a conditional branch mirroring the existing pattern—if
BREW_PREV_ZSTREAM_VERSION: extract_container_images(BREW_PREV_ZSTREAM_VERSION,
BREW_REPO, CONTAINER_LIST, args.dry_run)—so the extract_container_images
function is invoked for BREW_PREV_ZSTREAM_VERSION alongside the other versions.
---
Outside diff comments:
In `@test/bin/build_images.sh`:
- Around line 724-735: The container extraction block in build_images.sh misses
BREW_PREV_ZSTREAM_VERSION; add a guarded call matching the other BREW_* lines so
its images are extracted. Specifically, add a line like the existing guarded
lines using BREW_PREV_ZSTREAM_VERSION and BREW_REPO with
extract_container_images (e.g., [ -n "${BREW_PREV_ZSTREAM_VERSION}" ] &&
extract_container_images "${BREW_PREV_ZSTREAM_VERSION}" "${BREW_REPO}"
"${CONTAINER_LIST}"), placed with the other BREW_* checks so prev-zstream
release-info images are included.
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 112-126: The snippet fails to declare/initialize
BREW_PREV_ZSTREAM_VERSION which is required by
rhel96-bootc-brew-prev-zstream-with-optional.containerfile; add the missing
global and initialize it from the environment using common.get_env_var just like
the other vars (i.e., add a global BREW_PREV_ZSTREAM_VERSION and a line
BREW_PREV_ZSTREAM_VERSION = common.get_env_var('BREW_PREV_ZSTREAM_VERSION')
alongside the existing BREW_LATEST_ZSTREAM_VERSION / BREW_YMINUS1_VERSION /
BREW_YMINUS2_VERSION assignments).
---
Duplicate comments:
In `@test/bin/manage_build_cache.sh`:
- Around line 10-11: The script currently hardcodes AWS_BUCKET_NAME on the
second line which overrides the default and any environment-provided value;
remove the hardcoded assignment (the line setting
AWS_BUCKET_NAME="microshift-build-ushift-6073-cache-us-west-2") so that only the
parameterized assignment
AWS_BUCKET_NAME="${AWS_BUCKET_NAME:-microshift-build-cache}" remains, or if a
test bucket is needed keep it behind an explicit conditional or CI-only guard
that sets AWS_BUCKET_NAME only when a TEST_ENV variable is present.
---
Nitpick comments:
In `@test/bin/manage_brew_rpms.sh`:
- Around line 52-57: Inside the find_package function, ver_x and ver_y are not
declared local and leak to the caller; make them local by adding local
declarations for ver_x and ver_y (and any other temporary vars used for version
math, e.g., ver_prev_y if present) before assigning via cut and arithmetic so
the variables don't persist after the function returns.
- Around line 176-181: final_sub_dir may be left unset when ver_type is not
ec/rc/nightly/zstream; update the logic in the function that sets final_sub_dir
(used by brew_cli_download and referenced from action_download) to handle
unexpected ver_type values by either assigning a sensible default (e.g.,
final_sub_dir="${sub_dir}" or a normalized fallback) or explicitly failing with
an error message; ensure the code path covers the existing if/elif block and
adds an else branch that sets final_sub_dir or exits, so brew_cli_download never
returns with final_sub_dir unset.
- Around line 102-108: The variables package_latest, package_prev,
package_yminus1, and package_yminus2 in the zstream case of the case block
(where find_package is invoked) are not declared local and thus leak to outer
scope; fix by declaring them local before assignment (i.e., make package_latest,
package_prev, package_yminus1, package_yminus2 local in the zstream branch) so
they are scoped to the function/block that contains this case; also ensure
package_list remains correctly assigned after those local declarations.
- Around line 176-181: The variable final_sub_dir is being assigned inside the
conditional but not declared local, causing scope leakage; inside the function
that contains this block (the function that uses ver_type and sub_dir), add a
local declaration for final_sub_dir before its first assignment and then use
final_sub_dir in the existing conditional that formats sub_dir for EC/RC/nightly
(the block referencing ver_type, sub_dir and the sed transform) so the variable
is function-scoped and does not leak to the global environment.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (22)
test/assets/common_versions.sh.templatetest/bin/build_images.shtest/bin/build_rpms.shtest/bin/common.shtest/bin/common_versions.shtest/bin/manage_brew_rpms.shtest/bin/manage_build_cache.shtest/bin/pyutils/build_bootc_images.pytest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-prev-zstream-with-optional.containerfiletest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-y1-with-optional.containerfiletest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-y2-with-optional.containerfiletest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-z1-with-optional.containerfiletest/image-blueprints/layer4-release/group1/rhel96-brew-y2-with-optionals.tomltest/image-blueprints/layer4-release/group2/rhel96-brew-y1-with-optionals.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-lrel-optional.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-lrel-tuned.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-nightly-with-optionals.tomltest/image-blueprints/layer4-release/group4/rhel96-brew-lrel-optional.image-installertest/scenarios-bootc/releases/el96-prev-zstream@el96-lrel@standard1.shtest/scenarios-bootc/releases/el96-prev-zstream@el96-lrel@standard2.shtest/scenarios-bootc/releases/el96-z1@el96-lrel@standard1.shtest/scenarios-bootc/releases/el96-z1@el96-lrel@standard2.sh
| LATEST_ZSTREAM_VERSION="$(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V | tail -n1 || echo "")" | ||
| PREV_ZSTREAM_VERSION="$(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V | head -n1 || echo "")" |
There was a problem hiding this comment.
Edge case: single z-stream version results in identical LATEST and PREV.
When only one z-stream version exists in BREW_RPM_SOURCE, both LATEST_ZSTREAM_VERSION (using tail -n1) and PREV_ZSTREAM_VERSION (using head -n1) will resolve to the same version. This could cause issues in upgrade testing scenarios where upgrading from the same version to itself is not meaningful.
Consider adding validation or documentation noting this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/assets/common_versions.sh.template` around lines 152 - 153, The current
logic assigns LATEST_ZSTREAM_VERSION and PREV_ZSTREAM_VERSION from the same
find/sort pipeline which yields identical values when only one z-stream exists;
change the approach to first capture the sorted list of matches (from
BREW_RPM_SOURCE for pattern using MINOR_VERSION) into a variable/array, set
LATEST_ZSTREAM_VERSION to the last element and set PREV_ZSTREAM_VERSION to the
second-to-last element or to an empty value if the list length is 1 (so PREV is
not identical to LATEST), and ensure downstream code can handle an empty
PREV_ZSTREAM_VERSION.
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${MINOR_VERSION}" "${BREW_RPM_SOURCE}" "rc" || echo "WARNING: Failed to download RC RPMs for 4.${MINOR_VERSION}" | ||
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${MINOR_VERSION}" "${BREW_RPM_SOURCE}" "ec" || echo "WARNING: Failed to download EC RPMs for 4.${MINOR_VERSION}" | ||
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${MINOR_VERSION}" "${BREW_RPM_SOURCE}" "zstream" || echo "WARNING: Failed to download zstream RPMs for 4.${MINOR_VERSION}" | ||
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${MINOR_VERSION}" "${BREW_RPM_SOURCE}" "nightly" || echo "WARNING: Failed to download nightly RPMs for 4.${MINOR_VERSION}" | ||
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${PREVIOUS_MINOR_VERSION}" "${BREW_RPM_SOURCE}" "zstream" || echo "WARNING: Failed to download zstream RPMs for 4.${PREVIOUS_MINOR_VERSION}" | ||
| bash -x "${SCRIPTDIR}/manage_brew_rpms.sh" download "4.${YMINUS2_MINOR_VERSION}" "${BREW_RPM_SOURCE}" "zstream" || ( echo "WARNING: Failed to download zstream RPMs for 4.${YMINUS2_MINOR_VERSION}" && return 1 ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find YMINUS2 references and context
rg -n "YMINUS2" --type=sh -C3Repository: openshift/microshift
Length of output: 12388
🏁 Script executed:
#!/bin/bash
# Check the function context around lines 124-129
sed -n '100,140p' test/bin/build_rpms.sh | cat -nRepository: openshift/microshift
Length of output: 2679
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation in build_rpms.sh explaining the error handling
head -50 test/bin/build_rpms.shRepository: openshift/microshift
Length of output: 1719
Add comment explaining why Y-2 download failure exits the function while other downloads only warn.
The Y-2 zstream download (line 130) uses return 1 on failure, causing the entire function to exit with an error, while RC, EC, nightly, and Y-1 zstream downloads only emit warnings. If this asymmetry is intentional (Y-2 is mandatory for upgrade testing while others are optional), add an inline comment to clarify the design decision for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bin/build_rpms.sh` around lines 124 - 129, Add an inline comment next to
the failing Y-2 zstream download invocation (the bash call that uses
"${YMINUS2_MINOR_VERSION}" and ends with "&& return 1") explaining why this
download must fail the function while the other manage_brew_rpms.sh download
calls for RC, EC, nightly and Y-1 only emit warnings; reference that this is
intentional because the Y-2 zstream RPM is required for upgrade testing (or
whatever the policy is) so failure should abort, and briefly state that other
artifacts are optional so they only warn.
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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: 4
♻️ Duplicate comments (2)
test/bin/pyutils/build_bootc_images.py (1)
112-126:⚠️ Potential issue | 🟠 MajorMissing container image extraction for BREW_PREV_ZSTREAM_VERSION.
Prev‑zstream scenarios will fail if those images aren’t mirrored/extracted here.🐛 Suggested fix
@@ - global BREW_LATEST_ZSTREAM_VERSION - global BREW_YMINUS1_VERSION - global BREW_YMINUS2_VERSION + global BREW_LATEST_ZSTREAM_VERSION + global BREW_PREV_ZSTREAM_VERSION + global BREW_YMINUS1_VERSION + global BREW_YMINUS2_VERSION @@ - BREW_LATEST_ZSTREAM_VERSION = common.get_env_var('BREW_LATEST_ZSTREAM_VERSION') - BREW_YMINUS1_VERSION = common.get_env_var('BREW_YMINUS1_VERSION') - BREW_YMINUS2_VERSION = common.get_env_var('BREW_YMINUS2_VERSION') + BREW_LATEST_ZSTREAM_VERSION = common.get_env_var('BREW_LATEST_ZSTREAM_VERSION') + BREW_PREV_ZSTREAM_VERSION = common.get_env_var('BREW_PREV_ZSTREAM_VERSION') + BREW_YMINUS1_VERSION = common.get_env_var('BREW_YMINUS1_VERSION') + BREW_YMINUS2_VERSION = common.get_env_var('BREW_YMINUS2_VERSION') @@ if BREW_LATEST_ZSTREAM_VERSION: extract_container_images(BREW_LATEST_ZSTREAM_VERSION, BREW_REPO, CONTAINER_LIST, args.dry_run) + if BREW_PREV_ZSTREAM_VERSION: + extract_container_images(BREW_PREV_ZSTREAM_VERSION, BREW_REPO, CONTAINER_LIST, args.dry_run) if BREW_YMINUS1_VERSION: extract_container_images(BREW_YMINUS1_VERSION, BREW_REPO, CONTAINER_LIST, args.dry_run)Also applies to: 633-638
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/build_bootc_images.py` around lines 112 - 126, The script omits reading the BREW_PREV_ZSTREAM_VERSION env var so prev-zstream image mirroring will fail; add extraction similar to the other vars by calling common.get_env_var('BREW_PREV_ZSTREAM_VERSION') and assign it to a module-level variable named BREW_PREV_ZSTREAM_VERSION alongside BREW_LATEST_ZSTREAM_VERSION / BREW_YMINUS1_VERSION / BREW_YMINUS2_VERSION in the initialization block, and make the same addition in the later duplicate section around the 633-638 region so both places define BREW_PREV_ZSTREAM_VERSION consistently.test/assets/common_versions.sh.template (1)
145-185:⚠️ Potential issue | 🟠 MajorPREV_ZSTREAM_VERSION selects the oldest z‑stream, not Z‑1.
head -n1yields the earliest version. For Z‑1 upgrades you want the second newest (or empty if only one). Also note this file is generated; apply the fix in the generator.🐛 Suggested fix
-LATEST_ZSTREAM_VERSION="$(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V | tail -n1 || echo "")" -PREV_ZSTREAM_VERSION="$(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V | head -n1 || echo "")" +mapfile -t zstream_versions < <(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V) +if ((${`#zstream_versions`[@]} > 0)); then + LATEST_ZSTREAM_VERSION="${{zstream_versions[$((${`#zstream_versions`[@]}-1))]}}" +else + LATEST_ZSTREAM_VERSION="" +fi +if ((${`#zstream_versions`[@]} > 1)); then + PREV_ZSTREAM_VERSION="${{zstream_versions[$((${`#zstream_versions`[@]}-2))]}}" +else + PREV_ZSTREAM_VERSION="" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/assets/common_versions.sh.template` around lines 145 - 185, PREV_ZSTREAM_VERSION currently picks the oldest z-stream because it uses head -n1 after sorting; change its selection logic in the generator so it picks the second-newest z-stream (i.e., after sort -V take the last two and choose the earlier of those, or set empty if only one result) — update the variable PREV_ZSTREAM_VERSION in the template/generator to use the “second newest” selection (e.g., sort -V | tail -n2 | head -n1 semantics) and ensure downstream BREW_PREV_ZSTREAM_VERSION uses the updated PREV_ZSTREAM_VERSION unchanged.
🤖 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/common_versions.sh`:
- Around line 144-166: PREV_ZSTREAM_VERSION currently uses head -n1 which
returns the oldest patch instead of the previous-to-latest; change the selection
for PREV_ZSTREAM_VERSION to pick the second-to-last sorted entry (e.g., use sort
-V and select the penultimate element with a safe pipeline such as tail -n2 |
head -n1 or equivalent) and ensure it yields empty when only one version exists
so downstream BREW_PREV_ZSTREAM_VERSION (get_vrel_from_rpm on
"${BREW_RPM_SOURCE}/${PREV_ZSTREAM_VERSION}/${UNAME_M}/") behaves correctly.
In `@test/bin/common.sh`:
- Around line 407-412: MICROSHIFT_PREV_ZSTREAM_X86_64_RPMS_LIST is currently
populated from MICROSHIFT_Y2_X86_64_RPMS_LIST causing future drift; change the
assignment so MICROSHIFT_PREV_ZSTREAM_X86_64_RPMS_LIST is built from
MICROSHIFT_Y1_X86_64_RPMS_LIST (the same source used by the optional list) and
ensure MICROSHIFT_X86_64_RPMS_LIST continues to include the PREV_ZSTREAM list,
so any x86_64-only packages added to Y1 are propagated correctly.
In `@test/bin/manage_brew_rpms.sh`:
- Around line 34-88: In find_package(), preserve and restore the shell xtrace
state around the debugging suppression: detect current xtrace with a boolean
(e.g., xtrace_was_on=$([[ "$-" == *x* ]] && echo 1 || echo 0)), then call set +x
before the sensitive block and after the case restore with if [ "$xtrace_was_on"
-eq 1 ]; then set -x; fi (or just set +x only if xtrace was on and restore only
when it was), ensuring you reference the find_package function and the existing
set +x / set -x lines to wrap the section that queries repos and builds.
In `@test/scenarios-bootc/releases/el96-prev-zstream`@el96-lrel@standard1.sh:
- Around line 20-25: The current scenario_remove_vms function uses
exit_if_image_not_found which exits early (status 0) and can skip cleanup;
instead ensure cleanup always runs by removing early exits: change the image
checks to non-fatal probes (e.g. call a non-exiting helper or use
exit_if_image_not_found || true) and always execute remove_vm host1 (and any
pool cleanup steps) regardless of image probe result; update scenario_remove_vms
to perform image existence checks without exiting and then unconditionally call
remove_vm and pool cleanup so VMs/pools are never left behind.
---
Duplicate comments:
In `@test/assets/common_versions.sh.template`:
- Around line 145-185: PREV_ZSTREAM_VERSION currently picks the oldest z-stream
because it uses head -n1 after sorting; change its selection logic in the
generator so it picks the second-newest z-stream (i.e., after sort -V take the
last two and choose the earlier of those, or set empty if only one result) —
update the variable PREV_ZSTREAM_VERSION in the template/generator to use the
“second newest” selection (e.g., sort -V | tail -n2 | head -n1 semantics) and
ensure downstream BREW_PREV_ZSTREAM_VERSION uses the updated
PREV_ZSTREAM_VERSION unchanged.
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 112-126: The script omits reading the BREW_PREV_ZSTREAM_VERSION
env var so prev-zstream image mirroring will fail; add extraction similar to the
other vars by calling common.get_env_var('BREW_PREV_ZSTREAM_VERSION') and assign
it to a module-level variable named BREW_PREV_ZSTREAM_VERSION alongside
BREW_LATEST_ZSTREAM_VERSION / BREW_YMINUS1_VERSION / BREW_YMINUS2_VERSION in the
initialization block, and make the same addition in the later duplicate section
around the 633-638 region so both places define BREW_PREV_ZSTREAM_VERSION
consistently.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
test/assets/common_versions.sh.templatetest/bin/build_images.shtest/bin/build_rpms.shtest/bin/common.shtest/bin/common_versions.shtest/bin/manage_brew_rpms.shtest/bin/manage_build_cache.shtest/bin/pyutils/build_bootc_images.pytest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-prev-zstream-with-optional.containerfiletest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-y1-with-optional.containerfiletest/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-y2-with-optional.containerfiletest/image-blueprints/layer4-release/group1/rhel96-brew-y2-with-optionals.tomltest/image-blueprints/layer4-release/group2/rhel96-brew-y1-with-optionals.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-lrel-optional.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-lrel-tuned.tomltest/image-blueprints/layer4-release/group3/rhel96-brew-nightly-with-optionals.tomltest/image-blueprints/layer4-release/group4/rhel96-brew-lrel-optional.image-installertest/scenarios-bootc/releases/el96-prev-zstream@el96-lrel@standard1.shtest/scenarios-bootc/releases/el96-prev-zstream@el96-lrel@standard2.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- test/image-blueprints/layer4-release/group3/rhel96-brew-nightly-with-optionals.toml
- test/image-blueprints/layer4-release/group3/rhel96-brew-lrel-optional.toml
- test/bin/manage_build_cache.sh
- test/bin/build_images.sh
- test/image-blueprints-bootc/layer4-release/group1/rhel96-bootc-brew-prev-zstream-with-optional.containerfile
- test/image-blueprints/layer4-release/group4/rhel96-brew-lrel-optional.image-installer
- test/image-blueprints/layer4-release/group2/rhel96-brew-y1-with-optionals.toml
- test/bin/build_rpms.sh
- test/scenarios-bootc/releases/el96-prev-zstream@el96-lrel@standard2.sh
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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: 2
♻️ Duplicate comments (1)
test/bin/manage_brew_rpms.sh (1)
60-86: Preserve xtrace state when temporarily disabling tracing.As noted in a previous review,
set +x/set -xforces tracing on even if it wasn't enabled, which can add noisy logs. The fix was suggested but not implemented.✅ Suggested fix
+ local xtrace_state + xtrace_state="$(set +o | grep xtrace)" set +x case ${ver_type} in # ... case handling ... esac - set -x + eval "${xtrace_state}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 60 - 86, The script currently unconditionally runs set +x at the start and set -x at the end of the case, which forces xtrace on even if it was originally off; change this to save the original xtrace state before disabling tracing and restore it afterwards (e.g., detect if the shell had xtrace enabled via checking "$-" for "x" or using a temporary variable like XTRACE_ENABLED), replace the unconditional set +x with conditional disabling (only run set +x if xtrace was enabled) and replace the final set -x with conditional restore (only run set -x if it was enabled), updating the region around the existing set +x / set -x lines that bracket the case block.
🤖 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/manage_brew_rpms.sh`:
- Line 64: The sed expression that strips the architecture suffix in the
assignment to package_filtered uses ".${UNAME_M}$" which treats the dot as a
regex metacharacter; change the pattern in that sed call inside the
package_filtered pipeline (the sed "s/.${UNAME_M}$//" invocation) to escape the
literal dot so it only matches a dot before the arch (e.g., use a pattern
equivalent to "\.${UNAME_M}$"), and ensure the escaping works in the current
quoting context (e.g., double-quoted string should contain "\\.${UNAME_M}$").
In `@test/bin/pyutils/build_bootc_images.py`:
- Around line 124-126: The three variables BREW_PREV_ZSTREAM_VERSION,
BREW_YMINUS1_VERSION, and BREW_YMINUS2_VERSION currently call
common.get_env_var(...) with no default which will cause the script to exit if
the env vars are missing; change these calls to provide an empty-string default
(e.g., common.get_env_var('BREW_PREV_ZSTREAM_VERSION', '') ) so they match the
shell script behavior and do not abort when no matching brew versions exist;
update all three assignments (BREW_PREV_ZSTREAM_VERSION, BREW_YMINUS1_VERSION,
BREW_YMINUS2_VERSION) accordingly or alternatively adjust common.get_env_var to
return '' by default.
---
Duplicate comments:
In `@test/bin/manage_brew_rpms.sh`:
- Around line 60-86: The script currently unconditionally runs set +x at the
start and set -x at the end of the case, which forces xtrace on even if it was
originally off; change this to save the original xtrace state before disabling
tracing and restore it afterwards (e.g., detect if the shell had xtrace enabled
via checking "$-" for "x" or using a temporary variable like XTRACE_ENABLED),
replace the unconditional set +x with conditional disabling (only run set +x if
xtrace was enabled) and replace the final set -x with conditional restore (only
run set -x if it was enabled), updating the region around the existing set +x /
set -x lines that bracket the case block.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
test/assets/common_versions.sh.templatetest/bin/build_images.shtest/bin/common_versions.shtest/bin/manage_brew_rpms.shtest/bin/pyutils/build_bootc_images.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/bin/common_versions.sh
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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.
♻️ Duplicate comments (4)
test/assets/common_versions.sh.template (1)
155-157:⚠️ Potential issue | 🟠 MajorTemplate mirrors the PREV_ZSTREAM_VERSION selection bug.
This template uses the same
head -n1logic, so it will pick the oldest patch. Align it with the corrected “previous Z” selection.🛠️ Proposed fix
-PREV_ZSTREAM_VERSION="$(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V | head -n1 || echo "")" +mapfile -t ZSTREAM_VERSIONS < <(find "${{BREW_RPM_SOURCE}}" -maxdepth 1 -type d -regex ".*/[45]\.${{MINOR_VERSION}}\.[0-9]+$" -printf '%f\n' | sort -V) +PREV_ZSTREAM_VERSION="" +if ((${`#ZSTREAM_VERSIONS`[@]} == 1)); then + PREV_ZSTREAM_VERSION="${ZSTREAM_VERSIONS[0]}" +elif ((${`#ZSTREAM_VERSIONS`[@]} > 1)); then + PREV_ZSTREAM_VERSION="${ZSTREAM_VERSIONS[$(( ${`#ZSTREAM_VERSIONS`[@]} - 2 ))]}" +fi +unset ZSTREAM_VERSIONS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/assets/common_versions.sh.template` around lines 155 - 157, The template's PREV_ZSTREAM_VERSION assignment uses head -n1 so it selects the oldest patch instead of the latest; update the command that sets PREV_ZSTREAM_VERSION (in test/assets/common_versions.sh.template) to use the same selection logic as YMINUS1_VERSION/YMINUS2_VERSION (i.e., choose the newest matching directory by replacing the head -n1 behavior with the tail -n1 approach and keep the sort -V pipeline and fallback || echo "" intact) so PREV_ZSTREAM_VERSION picks the latest patch Z for the given minor version.test/bin/manage_brew_rpms.sh (2)
67-67:⚠️ Potential issue | 🟡 MinorEscape the dot in the arch-suffix strip regex.
.matches any character, sos/.${UNAME_M}$//can strip unintended suffixes.🐛 Proposed fix
- package_filtered=$(echo "${package_list}" | grep "microshift-0:" | sed 's/0://' | sed "s/.${UNAME_M}$//" | sort -V | uniq ) || true + package_filtered=$(echo "${package_list}" | grep "microshift-0:" | sed 's/0://' | sed "s/\.${UNAME_M}$//" | sort -V | uniq ) || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` at line 67, The sed expression used when building package_filtered incorrectly uses s/.${UNAME_M}$// which treats . as "any character" and can remove unintended characters; update that substitution (used in the pipeline that sets package_filtered from package_list and references UNAME_M) to escape the dot (e.g., use \.${UNAME_M}$) and ensure the backslash is properly quoted for the shell so the regex matches a literal dot before the architecture suffix.
63-90:⚠️ Potential issue | 🟡 MinorPreserve xtrace state when suppressing tracing.
set +x/set -xforces tracing on even if it was off originally. Capture and restore the prior state.✅ Proposed fix
- set +x + local xtrace_state + xtrace_state="$(set +o | grep xtrace)" + set +x @@ - set -x + eval "${xtrace_state}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 63 - 90, Save the current xtrace state before the existing set +x and restore it after the case block instead of unconditionally doing set -x: detect tracing with something like XTRACE_WAS_ON=$([[ $- == *x* ]] && echo 1 || echo 0) before set +x, keep the case that references ver_type/package_filtered/package, and replace the unconditional set -x with conditional restoration (if [[ $XTRACE_WAS_ON -eq 1 ]]; then set -x; fi) so tracing is only re-enabled if it was on originally.test/bin/common_versions.sh (1)
155-157:⚠️ Potential issue | 🟠 MajorPREV_ZSTREAM_VERSION selects the oldest patch, not Z-1.
With multiple z-streams,
head -n1yields the oldest version, which breaks the intended “previous Z” upgrade path. Select the second-to-latest version (or the only one if just one exists).🛠️ Proposed fix
-PREV_ZSTREAM_VERSION="$(find "${BREW_RPM_SOURCE}" -maxdepth 1 -type d -regex ".*/[45]\.${MINOR_VERSION}\.[0-9]+$" -printf '%f\n' | sort -V | head -n1 || echo "")" +mapfile -t ZSTREAM_VERSIONS < <(find "${BREW_RPM_SOURCE}" -maxdepth 1 -type d -regex ".*/[45]\.${MINOR_VERSION}\.[0-9]+$" -printf '%f\n' | sort -V) +PREV_ZSTREAM_VERSION="" +if ((${`#ZSTREAM_VERSIONS`[@]} == 1)); then + PREV_ZSTREAM_VERSION="${ZSTREAM_VERSIONS[0]}" +elif ((${`#ZSTREAM_VERSIONS`[@]} > 1)); then + PREV_ZSTREAM_VERSION="${ZSTREAM_VERSIONS[$(( ${`#ZSTREAM_VERSIONS`[@]} - 2 ))]}" +fi +unset ZSTREAM_VERSIONS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/common_versions.sh` around lines 155 - 157, PREV_ZSTREAM_VERSION currently picks the oldest patch because it uses head -n1; change its pipeline to sort -V and then select the second-to-last entry when multiple matches exist (otherwise return the single match). In practice, update the PREV_ZSTREAM_VERSION assignment that reads from BREW_RPM_SOURCE to use something like sort -V | tail -n2 | head -n1 with a fallback to the single-entry case so the variable yields the previous Z stream (or the only stream if one exists).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/assets/common_versions.sh.template`:
- Around line 155-157: The template's PREV_ZSTREAM_VERSION assignment uses head
-n1 so it selects the oldest patch instead of the latest; update the command
that sets PREV_ZSTREAM_VERSION (in test/assets/common_versions.sh.template) to
use the same selection logic as YMINUS1_VERSION/YMINUS2_VERSION (i.e., choose
the newest matching directory by replacing the head -n1 behavior with the tail
-n1 approach and keep the sort -V pipeline and fallback || echo "" intact) so
PREV_ZSTREAM_VERSION picks the latest patch Z for the given minor version.
In `@test/bin/common_versions.sh`:
- Around line 155-157: PREV_ZSTREAM_VERSION currently picks the oldest patch
because it uses head -n1; change its pipeline to sort -V and then select the
second-to-last entry when multiple matches exist (otherwise return the single
match). In practice, update the PREV_ZSTREAM_VERSION assignment that reads from
BREW_RPM_SOURCE to use something like sort -V | tail -n2 | head -n1 with a
fallback to the single-entry case so the variable yields the previous Z stream
(or the only stream if one exists).
In `@test/bin/manage_brew_rpms.sh`:
- Line 67: The sed expression used when building package_filtered incorrectly
uses s/.${UNAME_M}$// which treats . as "any character" and can remove
unintended characters; update that substitution (used in the pipeline that sets
package_filtered from package_list and references UNAME_M) to escape the dot
(e.g., use \.${UNAME_M}$) and ensure the backslash is properly quoted for the
shell so the regex matches a literal dot before the architecture suffix.
- Around line 63-90: Save the current xtrace state before the existing set +x
and restore it after the case block instead of unconditionally doing set -x:
detect tracing with something like XTRACE_WAS_ON=$([[ $- == *x* ]] && echo 1 ||
echo 0) before set +x, keep the case that references
ver_type/package_filtered/package, and replace the unconditional set -x with
conditional restoration (if [[ $XTRACE_WAS_ON -eq 1 ]]; then set -x; fi) so
tracing is only re-enabled if it was on originally.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
test/assets/common_versions.sh.templatetest/bin/common_versions.shtest/bin/manage_brew_rpms.sh
|
@agullon: This pull request references USHIFT-6073 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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.
♻️ Duplicate comments (1)
test/bin/manage_brew_rpms.sh (1)
63-89: Preserve xtrace state when temporarily disabling tracing.The
set +x/set -xpattern forces tracing on at the end even if it wasn't enabled before, which can add noisy logs. Consider capturing and restoring the prior state.♻️ Suggested fix
+ local xtrace_was_on=0 + [[ "$-" == *x* ]] && xtrace_was_on=1 set +x case ${ver_type} in # ... case logic ... esac - set -x + [[ "${xtrace_was_on}" -eq 1 ]] && set -x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/manage_brew_rpms.sh` around lines 63 - 89, The script temporarily disables xtrace with set +x before the case ${ver_type} in block and unconditionally reenables it with set -x, which forces tracing even if it was originally disabled; capture the prior xtrace state before set +x (e.g. detect whether the shell had xtrace enabled via "$-" or set -o) and restore that state after the case block instead of blindly calling set -x so the original tracing behavior is preserved for the rest of manage_brew_rpms.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/bin/manage_brew_rpms.sh`:
- Around line 63-89: The script temporarily disables xtrace with set +x before
the case ${ver_type} in block and unconditionally reenables it with set -x,
which forces tracing even if it was originally disabled; capture the prior
xtrace state before set +x (e.g. detect whether the shell had xtrace enabled via
"$-" or set -o) and restore that state after the case block instead of blindly
calling set -x so the original tracing behavior is preserved for the rest of
manage_brew_rpms.sh.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
test/bin/common.shtest/bin/manage_brew_rpms.shtest/bin/pyutils/build_bootc_images.py
agullon
force-pushed
the
USHIFT-6073
branch
3 times, most recently
from
3817f9b to
e2abbb7
Compare
pre-commit.check-secrets: ENABLED
|
/test e2e-aws-tests-cache |
|
@agullon: 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. |
|
/retest |
3 participants
This PR adds the capability to test Z-stream upgrades (e.g., 4.22.3 -> 4.22.7) in addition to the existing Y-stream upgrade tests (e.g., 4.21 -> 4.22).
Changes include:
Summary by CodeRabbit
New Features
Improvements
Chores