[Gating][Upgrade] Align cnv upgrade with konflux builds#4061
[Gating][Upgrade] Align cnv upgrade with konflux builds#4061rnetser merged 1 commit intoRedHatQE:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaces ICSP-based IDMS handling with Konflux-based IDMS flow: adds Konflux constants and utilities, a fixture to compute required Konflux mirrors, renames the ICSP fixture to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4061 +/- ##
==========================================
- Coverage 98.56% 98.19% -0.37%
==========================================
Files 25 25
Lines 2297 2382 +85
==========================================
+ Hits 2264 2339 +75
- Misses 33 43 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/install_upgrade_operators/product_upgrade/test_upgrade.py (2)
67-73:⚠️ Potential issue | 🟡 MinorLOW: Update docstring steps from ICSP terminology to Konflux IDMS terminology.
Line 70 still says “Generate a new ICSP…”, but this test path now uses Konflux IDMS. Please align the steps text to the current workflow to avoid confusion during triage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py` around lines 67 - 73, The docstring for the CNV upgrade test still refers to ICSP; update its step list to reflect Konflux IDMS terminology used by the current test path: replace the phrase "Generate a new ICSP for the IIB image being used." with something like "Generate a new Konflux IDMS entry for the IIB image being used." in the top-level docstring of tests/install_upgrade_operators/product_upgrade/test_upgrade.py (the descriptive block for the CNV upgrade process) so the steps accurately describe the current workflow.
49-65:⚠️ Potential issue | 🟠 MajorHIGH: Use
@pytest.mark.usefixturesforupdated_konflux_idmsinstead of an unused argument.Line 57 injects a fixture whose return value is not used. Keep the dependency as a fixture marker and remove the unused parameter.
Proposed fix
@@ `@pytest.mark.gating` `@pytest.mark.cnv_upgrade` `@pytest.mark.polarion`("CNV-2991") `@pytest.mark.dependency`(name=IUO_UPGRADE_TEST_DEPENDENCY_NODE_ID) + `@pytest.mark.usefixtures`("updated_konflux_idms") def test_cnv_upgrade_process( self, admin_client, hco_namespace, cnv_target_version, cnv_upgrade_stream, fired_alerts_before_upgrade, disabled_default_sources_in_operatorhub, - updated_konflux_idms, updated_custom_hco_catalog_source_image, updated_cnv_subscription_source, approved_cnv_upgrade_install_plan,As per coding guidelines: "ALWAYS use
@pytest.mark.usefixturesdecorator when fixture return value is not used by test".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py` around lines 49 - 65, The test_cnv_upgrade_process function currently accepts the updated_konflux_idms fixture but never uses its return value; remove updated_konflux_idms from the function signature and declare it as a fixture-only dependency by adding `@pytest.mark.usefixtures`("updated_konflux_idms") above the test_cnv_upgrade_process definition so the fixture still runs but its return value is not required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py`:
- Around line 67-73: The docstring for the CNV upgrade test still refers to
ICSP; update its step list to reflect Konflux IDMS terminology used by the
current test path: replace the phrase "Generate a new ICSP for the IIB image
being used." with something like "Generate a new Konflux IDMS entry for the IIB
image being used." in the top-level docstring of
tests/install_upgrade_operators/product_upgrade/test_upgrade.py (the descriptive
block for the CNV upgrade process) so the steps accurately describe the current
workflow.
- Around line 49-65: The test_cnv_upgrade_process function currently accepts the
updated_konflux_idms fixture but never uses its return value; remove
updated_konflux_idms from the function signature and declare it as a
fixture-only dependency by adding
`@pytest.mark.usefixtures`("updated_konflux_idms") above the
test_cnv_upgrade_process definition so the fixture still runs but its return
value is not required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24e64791-0e52-48bd-b8c3-c666b6cc1d5f
📒 Files selected for processing (5)
tests/install_upgrade_operators/product_upgrade/conftest.pytests/install_upgrade_operators/product_upgrade/test_upgrade.pytests/install_upgrade_operators/product_upgrade/utils.pyutilities/constants.pyutilities/operator.py
e6f8838 to
971c9ca
Compare
|
I already shared my opinion with you yesterday. I believe it is better to drop that IMDS for a good. This is very fragile, and you will have to adjust it again once the CI Release team drops the vX-YY part from the path in the future. |
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 (1)
tests/install_upgrade_operators/product_upgrade/test_upgrade.py (1)
67-72:⚠️ Potential issue | 🟡 MinorLOW: Update test docstring to match Konflux IDMS flow
The method still states “Generate a new ICSP.” After this change, the setup uses Konflux IDMS, so the step description is now stale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py` around lines 67 - 72, Update the test docstring in tests/install_upgrade_operators/product_upgrade/test_upgrade.py to reflect the Konflux IDMS flow: replace the stale step "Generate a new ICSP for the IIB image being used." with a concise description of the Konflux IDMS action performed (e.g., "Register or configure the IIB image in Konflux IDMS" or "Update Konflux IDMS to point to the IIB image being used"), keeping the surrounding steps unchanged and ensuring the docstring accurately matches the new setup flow used by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/install_upgrade_operators/product_upgrade/conftest.py`:
- Around line 106-107: The fixture function updated_konflux_idms currently
declares an unused fixture parameter cnv_image_name which forces unnecessary
setup; remove cnv_image_name from the updated_konflux_idms signature and any
internal references so the fixture no longer depends on that fixture, and update
any tests or fixture usages that explicitly pass cnv_image_name to
updated_konflux_idms to rely on the new signature (or import the proper fixtures
where actually needed). Ensure the function name updated_konflux_idms is kept
intact and run tests to confirm early-return paths (disconnected/hotfix) no
longer trigger cnv_image_name setup.
- Around line 99-103: The required_konflux_mirrors function returns an empty
list when target and current minors are equal because range(target.minor,
current.minor, -1) is exclusive of the end; change the range to be inclusive by
using range(target.minor, current.minor - 1, -1) so the mirror for the same
minor is produced (keep references to required_konflux_mirrors, target/current
Version, and KONFLUX_MIRROR_BASE_URL when locating the code).
In `@tests/install_upgrade_operators/product_upgrade/utils.py`:
- Around line 742-746: The current patch in the ResourceEditor call
unconditionally replaces spec.imageDigestMirrors for the IDMS named by
KONFLUX_IDMS_NAME, dropping any pre-existing mirror entries; instead, read the
current idms.spec.imageDigestMirrors (or idms.get()/idms.obj) and merge it with
image_digest_mirrors/required_mirrors (preserving existing entries and avoiding
duplicates, e.g., by keying on registry/source), then call
ResourceEditor(patches={idms: {"spec": {"imageDigestMirrors":
merged_list}}}).update() so the patch updates with the combined list rather than
overwriting.
---
Outside diff comments:
In `@tests/install_upgrade_operators/product_upgrade/test_upgrade.py`:
- Around line 67-72: Update the test docstring in
tests/install_upgrade_operators/product_upgrade/test_upgrade.py to reflect the
Konflux IDMS flow: replace the stale step "Generate a new ICSP for the IIB image
being used." with a concise description of the Konflux IDMS action performed
(e.g., "Register or configure the IIB image in Konflux IDMS" or "Update Konflux
IDMS to point to the IIB image being used"), keeping the surrounding steps
unchanged and ensuring the docstring accurately matches the new setup flow used
by the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67eb06f0-9092-4541-81c2-b6501ba39f5e
📒 Files selected for processing (3)
tests/install_upgrade_operators/product_upgrade/conftest.pytests/install_upgrade_operators/product_upgrade/test_upgrade.pytests/install_upgrade_operators/product_upgrade/utils.py
lukas-bednar
left a comment
There was a problem hiding this comment.
We need this change for all versions down to 4.12.
|
Manual cherry-pick is needed |
##### Short description: Old CPass idms won't work, align the konflux ##### More details: our cluster should already have idms ready. If some entry is missing - patch/create it. ##### What this PR does / why we need it: Gating is blocked - really urgent ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Upgrade tests now use Konflux-based image digest mirror sets for CNV upgrade verification instead of the previous ICSP approach. * Added fixtures and utilities to compute required mirror URLs and to verify/apply mirror configurations safely during upgrade flows. * Refactored test parameters for clearer, consistent orchestration of mirror checks and upgrade steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Harel Meir <hmeir@redhat.com>
|
Cherry-picked PR [Gating][Upgrade] Align cnv upgrade with konflux builds into cnv-4.19 |
##### Short description: Old CPass idms won't work, align the konflux ##### More details: our cluster should already have idms ready. If some entry is missing - patch/create it. ##### What this PR does / why we need it: Gating is blocked - really urgent ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Upgrade tests now use Konflux-based image digest mirror sets for CNV upgrade verification instead of the previous ICSP approach. * Added fixtures and utilities to compute required mirror URLs and to verify/apply mirror configurations safely during upgrade flows. * Refactored test parameters for clearer, consistent orchestration of mirror checks and upgrade steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Harel Meir <hmeir@redhat.com>
|
Cherry-picked PR [Gating][Upgrade] Align cnv upgrade with konflux builds into cnv-4.20 |
##### Short description: Old CPass idms won't work, align the konflux ##### More details: our cluster should already have idms ready. If some entry is missing - patch/create it. ##### What this PR does / why we need it: Gating is blocked - really urgent ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Upgrade tests now use Konflux-based image digest mirror sets for CNV upgrade verification instead of the previous ICSP approach. * Added fixtures and utilities to compute required mirror URLs and to verify/apply mirror configurations safely during upgrade flows. * Refactored test parameters for clearer, consistent orchestration of mirror checks and upgrade steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Harel Meir <hmeir@redhat.com>
|
Cherry-picked PR [Gating][Upgrade] Align cnv upgrade with konflux builds into cnv-4.21 |
|
@hmeir @lukas-bednar Please let me work on fixing the install and the eus_upgrade and will cherry-pick all changes at once. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
Move Konflux IDMS constants and functions (apply_konflux_idms, idms_has_all_mirrors) from product_upgrade/utils.py to shared utilities/operator.py. Replace legacy ICSP/IDMS fixtures in the install path with a new install_konflux_idms fixture, and replace EUS upgrade fixtures (created_eus_icsps, eus_applied_all_icsp) with eus_updated_konflux_idms. This aligns install and EUS paths with the Konflux migration done for upgrades in PR RedHatQE#4061. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CNV operator builds migrated from Brew to Konflux, so the install and EUS upgrade test paths need Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from both install and EUS upgrade conftest files, and replaces them with new Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) Follows up on PR RedHatQE#4061 which fixed the upgrade path only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CNV operator builds migrated from Brew to Konflux, so the install and EUS upgrade test paths need Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from both install and EUS upgrade conftest files, and replaces them with new Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) - Upgrade path: add missing client param to updated_konflux_idms Follows up on PR RedHatQE#4061 which fixed the upgrade path only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
##### Short description: Move Konflux IDMS constants and functions (apply_konflux_idms, idms_has_all_mirrors) from product_upgrade/utils.py to shared utilities/operator.py. Replace legacy ICSP/IDMS fixtures in the install path with a new install_konflux_idms fixture, and replace EUS upgrade fixtures (created_eus_icsps, eus_applied_all_icsp) with eus_updated_konflux_idms. This aligns install and EUS paths with the Konflux migration done for upgrades in PR #4061. ##### More details: Complements #4061 ##### What this PR does / why we need it: Fix the IDMS to use Konflux. ##### Which issue(s) this PR fixes: N/A ##### Special notes for reviewer: N/A ##### jira-ticket: https://issues.redhat.com/browse/CNV-81192 https://issues.redhat.com/browse/CNV-81195 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Modernized test infrastructure to use Konflux IDMS-based mirror management system. * Removed legacy mirror management test utilities. * **Refactor** * Reorganized utility functions and constants across test modules for improved fixture management and code maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…lux IDMS (#4076) Manual Cherry pick of #4070 and #4061 ##### Short description: CNV operator builds migrated from Brew to Konflux, so install, upgrade and EUS upgrade test paths now require Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from install, upgrade and EUS upgrade conftest files, and replaces them with Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - Upgrade path: updated_konflux_idms (product_upgrade/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) ##### More details: N/A ##### What this PR does / why we need it: N/A ##### Which issue(s) this PR fixes: N/A ##### Special notes for reviewer: N/A ##### jira-ticket: N/A Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…lux IDMS (#4076) Manual Cherry pick of #4070 and #4061 ##### Short description: CNV operator builds migrated from Brew to Konflux, so install, upgrade and EUS upgrade test paths now require Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from install, upgrade and EUS upgrade conftest files, and replaces them with Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - Upgrade path: updated_konflux_idms (product_upgrade/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) ##### More details: N/A ##### What this PR does / why we need it: N/A ##### Which issue(s) this PR fixes: N/A ##### Special notes for reviewer: N/A ##### jira-ticket: N/A Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…lux IDMS (#4076) Manual Cherry pick of #4070 and #4061 ##### Short description: CNV operator builds migrated from Brew to Konflux, so install, upgrade and EUS upgrade test paths now require Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from install, upgrade and EUS upgrade conftest files, and replaces them with Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - Upgrade path: updated_konflux_idms (product_upgrade/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) ##### More details: N/A ##### What this PR does / why we need it: N/A ##### Which issue(s) this PR fixes: N/A ##### Special notes for reviewer: N/A ##### jira-ticket: N/A Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…lux IDMS Manual Cherry pick of RedHatQE#4070 and RedHatQE#4061 CNV operator builds migrated from Brew to Konflux, so install, upgrade and EUS upgrade test paths now require Konflux IDMS instead of the legacy ICSP/IDMS generated via `oc adm catalog mirror`. This change consolidates Konflux IDMS helpers (apply_konflux_idms, idms_has_all_mirrors) into tests/install_upgrade_operators/utils.py, removes the legacy ICSP fixtures from install, upgrade and EUS upgrade conftest files, and replaces them with Konflux-based fixtures: - Install path: installed_konflux_idms (product_install/conftest.py) - Upgrade path: updated_konflux_idms (product_upgrade/conftest.py) - EUS upgrade path: eus_updated_konflux_idms (product_upgrade/conftest.py) N/A N/A N/A N/A N/A Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Short description:
Old CPass idms won't work, align the konflux
More details:
our cluster should already have idms ready.
If some entry is missing - patch/create it.
What this PR does / why we need it:
Gating is blocked - really urgent
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit