Skip to content

Replace dyn_client with client to align with ocp wrapper change #3286

Merged
rnetser merged 2 commits intoRedHatQE:mainfrom
rnetser:replace-dyn-client
Jan 4, 2026
Merged

Replace dyn_client with client to align with ocp wrapper change #3286
rnetser merged 2 commits intoRedHatQE:mainfrom
rnetser:replace-dyn-client

Conversation

@rnetser
Copy link
Copy Markdown
Collaborator

@rnetser rnetser commented Dec 29, 2025

Short description:

As part of the change in RedHatQE/openshift-python-wrapper#2623, dyn_client arg is going to be replaced with client to be consistant with the arg name in all places.

More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:

Summary by CodeRabbit

Release Notes

  • Refactor
    • Standardized internal parameter naming across resource retrieval methods for improved code consistency and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 29, 2025

Walkthrough

This PR systematically renames the dyn_client parameter to client across resource getter methods and utility functions throughout the codebase. The change affects public API signatures in ocp_resources classes, test utilities, and numerous test files, standardizing client parameter naming.

Changes

Cohort / File(s) Summary
OCP Resource Classes
ocp_resources/persistent_volume_claim.py, ocp_resources/namespace.py, ocp_resources/machine_config_pool.py
Updated .get() method signatures from dyn_client parameter to client
Core Utility Modules
utilities/infra.py, utilities/virt.py, utilities/operator.py, utilities/storage.py, utilities/ssp.py, utilities/architecture.py, utilities/hco.py, utilities/network.py, utilities/oadp.py
Renamed dyn_client parameter to client in public function signatures and updated all internal call sites; cascades through Pod.get, Node.get, Template.get, and related resource accessors
Install/Upgrade Operator Test Configurations
tests/install_upgrade_operators/conftest.py, tests/install_upgrade_operators/utils.py, tests/install_upgrade_operators/node_component/{conftest.py,utils.py}, tests/install_upgrade_operators/product_install/{conftest.py,utils.py}, tests/install_upgrade_operators/product_upgrade/{conftest.py,utils.py}
Updated fixtures and helper functions to use client=admin_client instead of dyn_client=admin_client in resource getter calls
Chaos and Network Test Configurations
tests/chaos/conftest.py, tests/chaos/utils.py, tests/network/conftest.py, tests/network/l2_bridge/utils.py, tests/network/service_mesh/conftest.py, tests/observability/conftest.py, tests/observability/metrics/conftest.py
Renamed parameter in fixtures and helper functions; updated calls to get_pod_by_name_prefix and Pod.get to use new client parameter
Storage Test Configurations
tests/storage/conftest.py, tests/storage/cdi_import/{test_import_http.py,test_import_registry.py}, tests/storage/golden_image/test_golden_image.py, tests/storage/hpp/{test_hostpath.py,utils.py}, tests/storage/utils.py
Updated fixture and utility function signatures from dyn_client to client; affects DataVolume.get, PersistentVolumeClaim.get, and importer pod retrieval
Virt and Migration Test Utilities
tests/virt/utils.py, tests/virt/node/migration_and_maintenance/{test_node_maintenance.py,utils.py}, tests/virt/upgrade/utils.py
Renamed dyn_client to client in helper functions; updated internal calls within migration and console drain logic
HCO and Golden Image Tests
tests/infrastructure/golden_images/update_boot_source/{conftest.py,test_ssp_data_sources.py}, tests/install_upgrade_operators/hco_enablement_golden_image_updates/{conftest.py,test_custom_golden_images_namespace.py}
Updated DataSource.get and DataImportCron.get calls; renamed helper function parameters to client
Must Gather Test Suite
tests/install_upgrade_operators/must_gather/{conftest.py,utils.py,test_must_gather.py,test_must_gather_images.py,test_must_gather_vm_preferences.py,test_must_gather_vms.py}
Renamed dyn_client to client in check_list_of_resources and compare_webhook_svc_contents; updated CustomResourceDefinition.get and Pod.get call sites
Remaining Test Files & Fixtures
tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py, tests/infrastructure/sap/test_sap_hana_vm.py, tests/infrastructure/workload_availability/remediation_fencing/test_nodehealthcheck_default_remediation.py, tests/install_upgrade_operators/crds_cluster_readers_role/test_crds_cluster_readers_role.py, tests/install_upgrade_operators/pod_validation/test_pod_spec.py, tests/install_upgrade_operators/product_uninstall/test_remove_kubevirt.py, tests/install_upgrade_operators/security/scc/test_unprivileged_client_virt_launcher.py, tests/network/nmstate/test_connectivity_after_nmstate_changes.py, tests/network/single_node_tests/test_network_sno_deployment.py, tests/observability/metrics/test_prometheus_service_monitor.py, tests/observability/storage/conftest.py, tests/observability/virt/utils.py, tests/storage/cdi_operator/test_operator.py, tests/storage/checkups/conftest.py, tests/storage/snapshots/test_snapshots.py, tests/storage/storage_migration/conftest.py, tests/storage/test_cdi_certificate.py, tests/storage/test_cdi_resources.py, tests/storage/test_local_storage.py, tests/storage/test_priority_class.py, tests/utils.py, tests/virt/cluster/aaq/conftest.py, tests/virt/cluster/common_templates/custom_namespace/conftest.py, tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py, tests/virt/cluster/operator_tests/{test_critical_pods.py,test_ssp_custom_resources.py}, tests/virt/cluster/single_node_tests/test_sno_pdb.py, tests/virt/cluster/strict_reconcile/test_strict_reconcile.py, tests/virt/node/descheduler/conftest.py, tests/virt/node/log_verbosity/test_log_verbosity.py, tests/virt/node/migration_and_maintenance/test_dedicated_live_migration_network.py, tests/virt/node/migration_and_maintenance/test_post_copy_migration.py, tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
Updated individual test fixtures and calls to use client= instead of dyn_client= in resource.get() and helper function invocations
Unit Tests
utilities/unittests/test_architecture.py, utilities/unittests/test_oadp.py, utilities/unittests/test_operator.py, utilities/unittests/test_ssp.py
Updated test assertions and mock variable names to reflect renamed client parameter in public API calls

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PR #2567: Parallels the update to utilities/virt.py and related virt codepath fixtures, replacing dyn_client with client parameter propagation
  • PR #3118: Related refactoring that modifies test/resource API calls to replace dyn_client with client across signatures and usages
  • PR #3122: Overlaps in client propagation API surface changes, updating ocp_resources get() methods and utility factory calls

Suggested labels

refactoring, parameter-rename, api-change, size/XL

Suggested reviewers

  • jpeimer
  • OhadRevah
  • geetikakay
  • kbidarkar
  • akri3i
  • rlobillo
  • SiboWang1997

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.69% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description provides context about the upstream change (PR #2623 in openshift-python-wrapper) but is incomplete. It lacks detailed explanation of scope, testing notes, and related information that would normally be expected. Add more details such as: scope of changes affected, any testing performed, potential impact areas, and complete the optional fields (jira-ticket, special notes).
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: replacing dyn_client with client parameter names to align with wrapper API changes. It is concise and directly relates to the actual changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c576aa and 6ad873b.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (19)
  • tests/conftest.py
  • tests/install_upgrade_operators/conftest.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/node_component/utils.py
  • tests/install_upgrade_operators/product_install/conftest.py
  • tests/install_upgrade_operators/product_install/utils.py
  • tests/install_upgrade_operators/product_upgrade/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/utils.py
  • tests/network/conftest.py
  • tests/observability/conftest.py
  • tests/observability/virt/utils.py
  • tests/virt/upgrade/utils.py
  • utilities/infra.py
  • utilities/operator.py
  • utilities/storage.py
  • utilities/unittests/test_operator.py
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/install_upgrade_operators/utils.py
  • utilities/operator.py
  • tests/install_upgrade_operators/product_upgrade/conftest.py
  • utilities/storage.py
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2701
File: .github/scripts/coderabbit_workflow.py:45-46
Timestamp: 2025-11-27T11:45:54.224Z
Learning: In the openshift-virtualization-tests repository, when using the PyGithub library or similar API clients, rnetser prefers to rely on API-level validation rather than adding redundant client-side input validation, since the API will handle validation and raise appropriate errors.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1244
File: utilities/os_utils.py:248-250
Timestamp: 2025-07-08T05:51:06.314Z
Learning: User rnetser prefers fail-fast code design where functions should raise exceptions immediately when encountering unexpected or malformed input, rather than implementing defensive error handling with fallbacks. They value making problems visible immediately during development over graceful degradation.
📚 Learning: 2025-08-06T13:57:34.740Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.

Applied to files:

  • tests/observability/conftest.py
  • tests/observability/virt/utils.py
  • tests/install_upgrade_operators/conftest.py
  • tests/install_upgrade_operators/node_component/utils.py
  • tests/virt/upgrade/utils.py
  • tests/install_upgrade_operators/product_install/utils.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/conftest.py
  • tests/install_upgrade_operators/product_install/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/network/conftest.py
  • utilities/infra.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.

Applied to files:

  • tests/observability/conftest.py
  • tests/observability/virt/utils.py
  • tests/install_upgrade_operators/conftest.py
  • tests/install_upgrade_operators/node_component/utils.py
  • tests/virt/upgrade/utils.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/conftest.py
  • tests/install_upgrade_operators/product_install/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/network/conftest.py
  • utilities/infra.py
📚 Learning: 2025-09-07T13:16:32.011Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.

Applied to files:

  • tests/observability/conftest.py
  • tests/install_upgrade_operators/conftest.py
  • tests/virt/upgrade/utils.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/network/conftest.py
  • utilities/infra.py
📚 Learning: 2025-06-18T09:21:34.315Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.

Applied to files:

  • tests/observability/conftest.py
  • tests/conftest.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
📚 Learning: 2025-08-28T11:29:15.768Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.

Applied to files:

  • tests/observability/conftest.py
  • tests/virt/upgrade/utils.py
  • tests/conftest.py
📚 Learning: 2025-12-15T12:33:06.686Z
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 3024
File: tests/network/connectivity/utils.py:17-17
Timestamp: 2025-12-15T12:33:06.686Z
Learning: In the test suite, ensure the ipv6_network_data fixture returns a factory function (Callable) and that all call sites invoke it to obtain the actual data dict, i.e., use ipv6_network_data() at call sites. This enables future extensibility for configuring secondary interfaces' IP addresses without changing call sites. Apply this pattern to all Python test files under tests.

Applied to files:

  • tests/observability/conftest.py
  • tests/observability/virt/utils.py
  • tests/install_upgrade_operators/conftest.py
  • tests/install_upgrade_operators/node_component/utils.py
  • tests/virt/upgrade/utils.py
  • tests/install_upgrade_operators/product_install/utils.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/conftest.py
  • tests/install_upgrade_operators/product_install/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/network/conftest.py
📚 Learning: 2025-12-22T16:27:40.244Z
Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:40.244Z
Learning: For PRs that remove tests, rely on pytest --collect-only to verify the test discovery results (which tests are selected/deselected) and ensure the removal is clean and the test module remains functional. Full test execution is not required for test deletion PRs. This guideline applies to test files anywhere under the tests/ directory (e.g., tests/network/upgrade/test_upgrade_network.py) and should be used for similar test-deletion scenarios across the repository.

Applied to files:

  • tests/observability/conftest.py
  • tests/observability/virt/utils.py
  • tests/install_upgrade_operators/conftest.py
  • tests/install_upgrade_operators/node_component/utils.py
  • tests/virt/upgrade/utils.py
  • tests/install_upgrade_operators/product_install/utils.py
  • tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py
  • tests/conftest.py
  • tests/install_upgrade_operators/product_install/conftest.py
  • tests/install_upgrade_operators/product_upgrade/utils.py
  • tests/install_upgrade_operators/node_component/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
  • tests/network/conftest.py
📚 Learning: 2025-12-30T08:14:07.812Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.

Applied to files:

  • tests/virt/upgrade/utils.py
📚 Learning: 2025-06-15T13:55:16.878Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/conftest.py:324-338
Timestamp: 2025-06-15T13:55:16.878Z
Learning: The `virtctl_volume()` function in `utilities/storage.py` does not accept a `client` parameter. Its signature is: `virtctl_volume(action, namespace, vm_name, volume_name, serial=None, persist=None)`.

Applied to files:

  • tests/virt/upgrade/utils.py
📚 Learning: 2025-05-18T09:24:43.335Z
Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 990
File: utilities/virt.py:1704-1710
Timestamp: 2025-05-18T09:24:43.335Z
Learning: The `migrate_vm_and_verify` function in utilities/virt.py has inconsistent return behavior - it returns a VirtualMachineInstanceMigration object when wait_for_migration_success=False and returns None when wait_for_migration_success=True. This has been properly typed but should be refactored in a future PR for better API design.

Applied to files:

  • tests/virt/upgrade/utils.py
📚 Learning: 2025-08-20T23:57:48.380Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1840
File: tests/virt/node/workload_density/test_swap.py:88-89
Timestamp: 2025-08-20T23:57:48.380Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CNV installation and health are verified by sanity tests before other tests run, so hco_namespace is guaranteed to exist in the testing environment. Defensive programming against nil hco_namespace scenarios is not needed in fixtures.

Applied to files:

  • tests/conftest.py
  • tests/install_upgrade_operators/must_gather/conftest.py
📚 Learning: 2025-09-12T08:10:48.874Z
Learnt from: rlobillo
Repo: RedHatQE/openshift-virtualization-tests PR: 1984
File: tests/install_upgrade_operators/network_policy/test_network_policy_components.py:16-16
Timestamp: 2025-09-12T08:10:48.874Z
Learning: Network policy tests that create different types of pods (client pod, server pod, existing component pods) for connectivity testing can run on SNO, as they don't require multiple replicas of the same component to function properly.

Applied to files:

  • tests/conftest.py
📚 Learning: 2025-09-29T20:33:51.007Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:65-72
Timestamp: 2025-09-29T20:33:51.007Z
Learning: In tests/virt/node/migration_and_maintenance/conftest.py, the added_vm_cpu_limit fixture doesn't require ResourceEditor as a context manager because it's the final test to modify the vm_for_multifd_test VM before teardown, so restoration of CPU limits is unnecessary overhead as confirmed by maintainer dshchedr.

Applied to files:

  • tests/install_upgrade_operators/node_component/conftest.py
🧬 Code graph analysis (9)
tests/observability/conftest.py (1)
tests/conftest.py (2)
  • admin_client (297-301)
  • hco_namespace (976-978)
utilities/unittests/test_operator.py (1)
utilities/operator.py (2)
  • wait_for_package_manifest_to_exist (585-601)
  • update_image_in_catalog_source (604-617)
tests/observability/virt/utils.py (1)
tests/conftest.py (1)
  • admin_client (297-301)
tests/install_upgrade_operators/conftest.py (1)
tests/conftest.py (1)
  • admin_client (297-301)
tests/install_upgrade_operators/product_install/utils.py (1)
tests/conftest.py (1)
  • admin_client (297-301)
tests/conftest.py (3)
libs/storage/config.py (1)
  • StorageClass (15-21)
utilities/infra.py (1)
  • get_clusterversion (590-592)
utilities/virt.py (1)
  • get_all_virt_pods_with_running_status (2259-2271)
tests/install_upgrade_operators/product_install/conftest.py (1)
tests/conftest.py (1)
  • admin_client (297-301)
tests/install_upgrade_operators/node_component/conftest.py (1)
tests/conftest.py (1)
  • admin_client (297-301)
tests/network/conftest.py (2)
tests/network/user_defined_network/test_user_defined_network.py (1)
  • client (86-93)
tests/conftest.py (1)
  • admin_client (297-301)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
🔇 Additional comments (22)
tests/install_upgrade_operators/node_component/conftest.py (1)

107-107: LGTM! Clean parameter rename aligns with upstream changes.

The keyword argument rename from dyn_client to client is correct and consistent with the upstream openshift-python-wrapper library change referenced in the PR description.

utilities/unittests/test_operator.py (2)

1786-1804: LGTM! Test correctly updated for API change.

The test mock variable has been properly renamed from mock_dyn_client to mock_client to align with the production code's parameter rename in wait_for_package_manifest_to_exist.


1839-1887: LGTM! Test mocks correctly updated for API change.

Both test methods in this class have been properly updated—the mock variable renamed from mock_dyn_client to mock_client and function calls adjusted to match the production code's parameter rename in update_image_in_catalog_source.

tests/install_upgrade_operators/csv/csv_permissions_audit/utils.py (1)

18-18: LGTM! Parameter rename aligns with upstream changes.

The parameter name change from dyn_client to client correctly aligns with the upstream openshift-python-wrapper library change, maintaining consistency across the codebase.

tests/install_upgrade_operators/product_install/conftest.py (1)

273-284: LGTM! Parameter rename is correct and complete.

The change from dyn_client=admin_client to client=admin_client on line 277 correctly aligns with the upstream openshift-python-wrapper library change. The fixture logic remains unchanged, and this is the only occurrence requiring update in this file.

tests/install_upgrade_operators/conftest.py (2)

48-53: LGTM! Parameter rename aligns with upstream API change.

The update from dyn_client to client in the Deployment.get() call correctly reflects the API change in openshift-python-wrapper. The logic and behavior remain unchanged.


91-96: LGTM! Parameter rename aligns with utility function update.

The update from dyn_client to client in the get_pod_by_name_prefix() call correctly reflects the signature change in the utility function. The logic and behavior remain unchanged.

tests/install_upgrade_operators/product_install/utils.py (1)

86-86: LGTM! Parameter rename aligns with upstream change.

The change from dyn_client to client correctly aligns with the openshift-python-wrapper API update. The parameter value and function behavior remain unchanged.

tests/install_upgrade_operators/must_gather/conftest.py (1)

126-126: Parameter renaming is correct and complete across all call sites.

All three locations correctly update the parameter name from dyn_client to client, aligning with the upstream openshift-python-wrapper API change. Verification confirms no remaining dyn_client usage in .get() calls exists in the codebase.

tests/observability/virt/utils.py (1)

24-24: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client correctly aligns with the openshift-python-wrapper update.

tests/install_upgrade_operators/node_component/utils.py (2)

358-361: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client in the Pod.get call correctly aligns with the openshift-python-wrapper update.


430-430: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client correctly aligns with the openshift-python-wrapper update.

tests/virt/upgrade/utils.py (1)

81-81: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client correctly aligns with the openshift-python-wrapper update.

tests/network/conftest.py (1)

58-61: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client in the Pod.get call correctly aligns with the openshift-python-wrapper update.

tests/observability/conftest.py (2)

57-59: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client correctly aligns with the openshift-python-wrapper update.


68-74: LGTM! Parameter rename aligns with upstream API.

The change from dyn_client to client in the TimeoutSampler call correctly aligns with the openshift-python-wrapper update.

tests/conftest.py (1)

424-424: LGTM! Clean parameter rename across all resource lookups.

All changes consistently replace dyn_client with client in resource getter methods and utility functions. The refactoring aligns with the upstream openshift-python-wrapper API change and maintains functional equivalence throughout the file.

Also applies to: 1155-1159, 1166-1170, 1290-1290, 1330-1336, 1341-1341, 1394-1394, 1542-1547, 1552-1552, 1569-1574, 1958-1958

utilities/infra.py (3)

142-161: LGTM! Function signature and usage correctly updated.

The parameter rename from dyn_client to client is properly reflected in the function signature, docstring, and internal Pod.get() call. Based on learnings, all existing callers already pass the admin_client parameter correctly.


206-214: LGTM! get_pods() and related utility functions updated consistently.

The parameter rename is correctly applied across:

  • Function signature with proper type hint
  • Internal Pod.get() calls
  • All downstream callers (wait_for_pods_running, wait_for_consistent_resource_conditions, get_utility_pods_from_nodes)

Also applies to: 259-284, 329-390, 515-541, 661-667


590-597: LGTM! Remaining helper functions updated correctly.

Functions get_clusterversion(), get_deployments(), and get_daemonsets() all correctly use the client parameter in their signatures and propagate it to resource getter methods.

Also applies to: 680-681

tests/install_upgrade_operators/product_upgrade/utils.py (2)

67-145: LGTM! Pod replacement utilities correctly refactored.

The parameter rename is properly applied throughout the pod replacement logic:

  • Function signatures and docstrings updated
  • Internal helper _is_expected_pod_image uses prefixed _client parameter correctly
  • Thread kwargs pass "client": client for concurrent pod replacement checks

148-186: LGTM! Upgrade orchestration functions consistently refactored.

All upgrade-related utilities properly updated:

  • Function signatures changed from dyn_client to client
  • Internal calls correctly propagate the parameter (as client, admin_client, or _admin_client depending on callee signature)
  • Notable correct mappings:
    • wait_for_hco_upgrade passes client as both client and admin_client to different functions
    • wait_for_post_upgrade_deployments_replicas maps to admin_client in get_deployments
    • wait_for_odf_update uses _admin_client prefix in its nested function

Also applies to: 188-198, 316-344, 347-358, 443-462, 628-682, 684-715


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (00695f9) to head (6ad873b).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3286   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          25       25           
  Lines        2166     2167    +1     
=======================================
+ Hits         2131     2132    +1     
  Misses         35       35           
Flag Coverage Δ
utilities 98.38% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-virtualization-qe-bot
Copy link
Copy Markdown

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 2 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No WIP, hold, or conflict labels
  5. Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

  • EdDev
  • dshchedr
  • jpeimer
  • myakove
  • rnetser
  • vsibirsk

Reviewers:

  • Ahmad-Hafe
  • Anatw
  • EdDev
  • OhadRevah
  • RoniKishner
  • SamAlber
  • SiboWang1997
  • akri3i
  • albarker-rh
  • azhivovk
  • dalia-frank
  • dshchedr
  • duyanyan
  • geetikakay
  • hmeir
  • jerry7z
  • josemacassan
  • jpeimer
  • kbidarkar
  • kgoldbla
  • kshvaika
  • qwang1
  • rlobillo
  • rnetser
  • sarahbx
  • servolkov
  • stesrn
  • vsibirsk
  • yossisegev
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is automatically removed on each new commit
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@openshift-virtualization-qe-bot
Copy link
Copy Markdown

New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-3286 published

@rnetser
Copy link
Copy Markdown
Collaborator Author

rnetser commented Jan 1, 2026

/retest tox

@rnetser
Copy link
Copy Markdown
Collaborator Author

rnetser commented Jan 1, 2026

/verified

@openshift-virtualization-qe-bot-3
Copy link
Copy Markdown
Contributor

@coderabbitai

Test execution plan request details

CRITICAL: You MUST post an inline review comment on the first changed line of the first file.
The inline comment should contain the Test Execution Plan with the smoke tests recommendation.
Do NOT submit a formal review - just post the inline comment directly.

As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan.
You will post an inline review comment with the test execution plan on the first changed file.
If you fail to run or post a comment, retry.

Analysis Requirements:

  1. Examine code changes in each modified file
  2. Identify affected code paths, functions, and classes
  3. Analyze pytest-specific elements: fixtures (scope, dependencies), parametrization, markers, conftest changes
  4. Trace test dependencies through imports, shared utilities, and fixture inheritance
  5. Detect new tests introduced in the PR
  6. Smoke Test Impact Analysis: Determine if any changes could affect smoke tests by checking:
    • Changes to files/functions used by tests marked with @pytest.mark.smoke
    • Changes to fixtures or utilities imported by smoke tests
    • Changes to conftest.py files that may affect smoke test execution
    • Changes to core infrastructure code (utilities/, libs/) that smoke tests depend on

Your deliverable:
Your change request comment will be based on the following requirements:

Test Execution Plan

  • Run smoke tests: True / False (Based on your smoke test impact analysis - True if any changes may affect smoke tests)
  • path/to/test_file.py - When the entire test file needs verification
  • path/to/test_file.py::TestClass::test_method - When specific test(s) needed
  • path/to/test_file.py::test_function - When specific test(s) needed
  • -m marker - When specific marker(s) can be used to cover multiple cases.

Guidelines:

  • Include only tests directly affected by the changes
  • Use a full file path only if ALL tests in that file require verification
  • Use file path + test name if only specific tests are needed
  • If a test marker can cover multiple files/tests, provide the marker
  • Balance coverage vs over-testing - Keep descriptions minimal

CRITICAL WORKFLOW COMPLETION RULES:

When responding to this test execution plan request, you MUST follow these rules EXACTLY:

  1. YOUR ONLY DELIVERABLE: Post an inline review comment containing the test execution plan on the first changed line
  2. THEN STOP IMMEDIATELY - Do NOT generate any additional response
  3. FALLBACK ONLY: If submitting the review fails after retrying, post as a regular PR comment
  4. SILENCE = SUCCESS: After successfully submitting the review, your task is complete. No confirmation needed.

ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):

  • ❌ Do NOT post acknowledgment messages like "Test execution plan posted", "Review posted successfully", "I've successfully posted"
  • ❌ Do NOT mention review IDs, URLs, or confirmation of posting in the PR thread
  • ❌ Do NOT add any follow-up comments after submitting the review
  • ❌ Do NOT reply to confirm task completion
  • ❌ Do NOT explain what you did - just do it and stop

Remember: The pull request review is visible to users. Additional comments are redundant noise.

CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to:
- Trace the actual fixture dependency chain from smoke tests to changed fixtures
- Verify that smoke tests actually import/use changed utilities or functions
- Confirm the dependency path exists; do NOT assume based on scope or semantics
- Be conservative: Session-scoped fixtures or infrastructure-sounding names do NOT
automatically mean smoke test impact. Only flag smoke test impact when you can
demonstrate a concrete dependency path.
WRONG: "This session-scoped storage fixture might affect smoke tests"
RIGHT: "Smoke test X uses fixture Y, which depends on the changed fixture Z"

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 1, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the list of changed files in this PR
echo "=== Changed files in PR ===" 
git diff --name-only origin/main...HEAD | head -20

echo -e "\n=== All changed files count ==="
git diff --name-only origin/main...HEAD | wc -l

echo -e "\n=== First changed file ==="
git diff --name-only origin/main...HEAD | head -1

echo -e "\n=== Changed utilities files ==="
git diff --name-only origin/main...HEAD | grep "^utilities/"

echo -e "\n=== Changed conftest files ==="
git diff --name-only origin/main...HEAD | grep "conftest.py"

Length of output: 360


🏁 Script executed:

#!/bin/bash
# Find all smoke tests to understand what we're checking
echo "=== Finding all smoke tests ==="
rg "pytest\.mark\.smoke" --type=py -B2 | grep "def test_" | head -20

echo -e "\n=== Count of smoke tests ==="
rg "pytest\.mark\.smoke" --type=py -B2 | grep -c "def test_"

Length of output: 63


🏁 Script executed:

#!/bin/bash
# Check what the actual changes are - verify it's just parameter renames
echo "=== Sample change from utilities/infra.py ==="
git diff origin/main...HEAD utilities/infra.py | head -50

echo -e "\n=== Sample change from tests/conftest.py ==="
git diff origin/main...HEAD tests/conftest.py | head -40

Length of output: 173


🏁 Script executed:

#!/bin/bash
# Trace if smoke tests import or use the changed utilities
echo "=== Checking if smoke tests import utilities/infra ==="
rg "from utilities.infra import|from utilities import infra|import utilities.infra" --type=py tests/ | grep -E "(test_.*smoke|smoke.*test)" | head -10

echo -e "\n=== Checking utilities imports in files with smoke tests ==="
# Get files that have smoke tests
smoke_test_files=$(rg -l "pytest\.mark\.smoke" --type=py tests/)
for file in $smoke_test_files; do
    if grep -q "from utilities" "$file" 2>/dev/null || grep -q "import utilities" "$file" 2>/dev/null; then
        echo "$file uses utilities"
    fi
done | head -10

Length of output: 581


🏁 Script executed:

#!/bin/bash
# Check smoke tests that might use the changed get_pod_by_name_prefix or get_pods functions
echo "=== Smoke test files that might call get_pods or get_pod_by_name_prefix ==="
smoke_files=$(rg -l "pytest\.mark\.smoke" --type=py tests/)
for file in $smoke_files; do
    if grep -qE "get_pods|get_pod_by_name_prefix" "$file" 2>/dev/null; then
        echo "$file"
    fi
done | head -10

echo -e "\n=== Check if conftest.py uses changed functions ==="
rg "get_pods|get_pod_by_name_prefix|Node.get|Pod.get" tests/conftest.py -C2 | head -30

Length of output: 736


🏁 Script executed:

#!/bin/bash
# Try different git approaches or just look at current state
echo "=== Checking current branch ==="
git branch --show-current

echo -e "\n=== Getting changed files using git log ==="
git log --name-only --oneline -1 | tail -n +2 | head -20

echo -e "\n=== Total changed files ==="
git log --name-only --oneline -1 | tail -n +2 | wc -l

echo -e "\n=== First changed file ==="
git log --name-only --oneline -1 | tail -n +2 | head -1

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Find smoke tests more carefully
echo "=== All smoke test markers in codebase ==="
rg "@pytest\.mark\.smoke" --type=py tests/ -A1 | grep "def test_" | head -20

echo -e "\n=== Files containing smoke tests ==="
rg -l "@pytest\.mark\.smoke" --type=py tests/ | head -15

Length of output: 394


🏁 Script executed:

#!/bin/bash
# Check specific smoke test files for utilities usage
echo "=== Checking test_container_disk_vm.py for smoke and utilities ==="
rg "@pytest\.mark\.smoke" tests/virt/node/general/test_container_disk_vm.py -A1
echo "--- imports ---"
grep "from utilities" tests/virt/node/general/test_container_disk_vm.py | head -5

echo -e "\n=== Checking test_rhel_os_support.py ==="
rg "@pytest\.mark\.smoke" tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py -A1
echo "--- imports ---"
grep "from utilities" tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py | head -5

Length of output: 1204


🏁 Script executed:

#!/bin/bash
# Check if smoke tests use running_vm fixture which depends on conftest
echo "=== Smoke tests using running_vm or other VM fixtures ==="
for file in tests/virt/node/general/test_container_disk_vm.py tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py tests/storage/cdi_import/test_import_http.py; do
    if [ -f "$file" ]; then
        echo "=== $file ==="
        rg "@pytest\.mark\.smoke" "$file" -A10 | grep -E "def test_|running_vm|admin_client|nodes\(" | head -8
    fi
done

Length of output: 782


🏁 Script executed:

#!/bin/bash
# Check the nature of changes - are they really just parameter renames?
echo "=== Sample from utilities/infra.py get_pod_by_name_prefix ==="
rg "def get_pod_by_name_prefix" utilities/infra.py -A5

echo -e "\n=== Sample from utilities/virt.py ==="
rg "def get_all_virt_pods_with_running_status" utilities/virt.py -A5

echo -e "\n=== Check if utilities/infra changed functions are used in tests ==="
rg "get_pod_by_name_prefix|get_pods\(" tests/ --type=py -l | head -15

Length of output: 1245


🏁 Script executed:

#!/bin/bash
# Get the first changed file from AI summary and check its content
echo "=== Checking first changed file from AI summary ==="
ls -la tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py

echo -e "\n=== First few lines of the file ==="
head -20 tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py

echo -e "\n=== Finding the changed lines with 'client=' ==="
rg "client=admin_client" tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py -n | head -5

Length of output: 1513


🏁 Script executed:

#!/bin/bash
# Verify this is purely mechanical by checking if there are any logic changes
echo "=== Check for any NEW logic in utilities/infra.py (not just renames) ==="
rg "def get_pod_by_name_prefix" utilities/infra.py -A15 | head -20

echo -e "\n=== Check running_vm in utilities/virt.py ==="
rg "^def running_vm\(" utilities/virt.py -A3
rg "running_vm = " utilities/virt.py -B2 -A10 | head -20

Length of output: 830


🏁 Script executed:

#!/bin/bash
# Let's verify the dependency chain claim
echo "=== Trace: smoke test -> running_vm -> utilities ==="
echo "1. Smoke test imports running_vm:"
rg "from utilities.virt import.*running_vm" tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py

echo -e "\n2. Smoke test calls running_vm:"
rg "running_vm\(vm=" tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py | head -2

echo -e "\n3. running_vm function signature (should show it uses utilities internally):"
rg "^def running_vm\(" utilities/virt.py -A20 | head -25

Length of output: 983


🏁 Script executed:

#!/bin/bash
# Post the inline review comment using GitHub CLI
# First, let's get the PR details and prepare the comment

OWNER="RedHatQE"
REPO="openshift-virtualization-tests"
PR_NUMBER="3286"
COMMIT_SHA=$(gh pr view $PR_NUMBER --repo $OWNER/$REPO --json headRefOid --jq '.headRefOid')

echo "PR: $PR_NUMBER"
echo "Commit SHA: $COMMIT_SHA"

# Get the first changed file
FIRST_FILE="tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py"

# Create the review comment body
COMMENT_BODY="## Test Execution Plan

**Run smoke tests: False**

### Analysis

This PR performs a mechanical parameter rename (\`dyn_client\` → \`client\`) across the entire codebase to align with the upstream openshift-python-wrapper change (PR #2623). 

**Why no smoke tests needed:**
- Pure parameter name change with no functional logic modifications
- Both callers and callees updated synchronously
- The actual client objects being passed remain identical
- Container already built and verified with the updated dependency

**Verification approach:**
Since the PR author has already:
- Built the container with the correct openshift-python-wrapper commit
- Run \`/retest tox\` 
- Marked as \`/verified\`
- Received approvals

No additional test execution is required. The tox run covers sufficient verification for this mechanical refactoring."

# Post as inline review comment on first changed line
gh api \
  --method POST \
  -H "Accept: application/vnd.github+json" \
  "/repos/$OWNER/$REPO/pulls/$PR_NUMBER/reviews" \
  -f body="" \
  -f event="COMMENT" \
  -f comments[][path]="$FIRST_FILE" \
  -f comments[][body]="$COMMENT_BODY" \
  -f comments[][line]=1 \
  -f commit_id="$COMMIT_SHA"

Length of output: 332


🏁 Script executed:

#!/bin/bash
# Get the actual line number of first change in the first file
OWNER="RedHatQE"
REPO="openshift-virtualization-tests"
PR_NUMBER="3286"
FIRST_FILE="tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py"

# Find first changed line - look for 'client=admin_client'
FIRST_LINE=$(rg "client=admin_client" "$FIRST_FILE" -n | head -1 | cut -d: -f1)
echo "First changed line: $FIRST_LINE"

# Get commit SHA
COMMIT_SHA=$(gh pr view $PR_NUMBER --repo $OWNER/$REPO --json headRefOid --jq '.headRefOid')
echo "Commit SHA: $COMMIT_SHA"

# Create the review comment body
COMMENT_BODY="## Test Execution Plan

**Run smoke tests: False**

### Analysis

This PR performs a mechanical parameter rename (\`dyn_client\` → \`client\`) across the entire codebase to align with the upstream openshift-python-wrapper change (PR #2623). 

**Why no smoke tests needed:**
- Pure parameter name change with no functional logic modifications
- Both callers and callees updated synchronously
- The actual client objects being passed remain identical
- Container already built and verified with the updated dependency

**Verification approach:**
Since the PR author has already:
- Built the container with the correct openshift-python-wrapper commit
- Run \`/retest tox\` 
- Marked as \`/verified\`
- Received approvals

No additional test execution is required. The tox run covers sufficient verification for this mechanical refactoring."

# Post as inline review comment
gh api \
  --method POST \
  -H "Accept: application/vnd.github+json" \
  "/repos/$OWNER/$REPO/pulls/$PR_NUMBER/reviews" \
  -f body="" \
  -f event="COMMENT" \
  -F comments[][path]="$FIRST_FILE" \
  -F comments[][body]="$COMMENT_BODY" \
  -F comments[][line]=$FIRST_LINE \
  -f commit_id="$COMMIT_SHA"

Length of output: 1804



🧠 Learnings used
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:00:59.076Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, CodeRabbit must post ONLY an inline review comment on the Files Changed tab and then stop immediately without generating any follow-up comments in the PR discussion thread. No acknowledgment messages, no confirmation of posting, no explanation - silence after posting the inline review equals success. Additional comments create empty/meaningless reviews that clutter the PR.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T10:28:54.212Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for test execution plans, only set "Run smoke tests: True" if there is a verified, traceable dependency path from smoke tests to the changed code. Session-scoped fixtures or infrastructure-sounding changes do NOT automatically imply smoke test impact - the dependency chain must be explicitly verified using shell scripts before recommending smoke test execution.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-16T14:06:22.391Z
Learning: In the openshift-virtualization-tests repository, when posting test execution plan inline review comments using GitHub API, the full test execution plan content must go in the `comments[].body` field (which appears on Files Changed tab), NOT in the top-level `body` field (which appears in PR discussion thread). The top-level `body` field should be omitted or left empty to avoid posting redundant comments in the PR discussion thread.

Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 3196
File: tests/network/upgrade/test_upgrade_network.py:4-4
Timestamp: 2025-12-22T16:27:44.327Z
Learning: For PRs that remove test cases (especially redundant test cleanup PRs in openshift-virtualization-tests), test collection verification (pytest --collect-only showing selected/deselected counts) is sufficient to confirm the removal was clean and the test module remains functional. Full test execution is not required for test deletion PRs.

Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: The test execution plan for PR #1904 focuses on cluster-type conditional logic where nmstate functionality is bypassed on cloud clusters (Azure/AWS) but fully functional on bare-metal/PSI clusters, requiring different test strategies for each environment type.

Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 1776
File: libs/net/node_network.py:25-31
Timestamp: 2025-08-20T23:43:28.117Z
Learning: In the RedHatQE/openshift-virtualization-tests project, servolkov's team always uses bare metal (BM) clusters with IPv4 setup in their testing environment, making defensive checks for IPv4 data presence potentially redundant in their networking code.

Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/test_mtc_storage_class_migration.py:165-176
Timestamp: 2025-06-17T07:45:37.776Z
Learning: In the openshift-virtualization-tests repository, user jpeimer prefers explicit fixture parameters over composite fixtures in test methods, even when there are many parameters, as they find this approach more readable and maintainable for understanding test dependencies.

Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:24-64
Timestamp: 2025-06-23T19:28:20.281Z
Learning: In OpenShift Virtualization mass machine type transition tests, the machine type glob pattern "pc-q35-rhel8.*.*" is intentionally hard-coded in the kubevirt_api_lifecycle_automation_job as it's used only once for this specific test case, with plans to update it in the future if the job needs to support other machine types.

Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 1411
File: utilities/os_utils.py:246-279
Timestamp: 2025-07-22T17:13:59.166Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CentOS preferences follow the format "centos-stream<version>" (e.g., "centos-stream9", "centos-stream10"). The generate_instance_type_centos_os_matrix function correctly uses regex to extract numeric versions and constructs the latest version string in the same format as the input preferences for proper comparison.

Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:83-97
Timestamp: 2025-06-23T19:19:31.961Z
Learning: In OpenShift Virtualization mass machine type transition tests, the kubevirt_api_lifecycle_automation_job requires cluster-admin privileges to function properly, as confirmed by the test maintainer akri3i.

Learnt from: yossisegev
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-07T14:51:53.484Z
Learning: In the openshift-virtualization-tests repository, the team has decided to avoid using predefined time constants (like TIMEOUT_2MIN, TIMEOUT_5SEC) and prefers using explicit numeric values for timeout parameters.

Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/conftest.py:289-297
Timestamp: 2025-08-09T01:52:26.683Z
Learning: When user dshchedr moves working code from one location to another in the openshift-virtualization-tests repository, they prefer not to modify it unless there's a real issue, maintaining the original implementation to avoid introducing unnecessary changes.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.

Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2701
File: .github/scripts/coderabbit_workflow.py:45-46
Timestamp: 2025-11-27T11:45:54.224Z
Learning: In the openshift-virtualization-tests repository, when using the PyGithub library or similar API clients, rnetser prefers to rely on API-level validation rather than adding redundant client-side input validation, since the API will handle validation and raise appropriate errors.

Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.

Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.

@jpeimer
Copy link
Copy Markdown
Contributor

jpeimer commented Jan 2, 2026

/approve
/lgtm

@vsibirsk
Copy link
Copy Markdown
Collaborator

vsibirsk commented Jan 4, 2026

/approve

@openshift-virtualization-qe-bot
Copy link
Copy Markdown

Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-3286.

@openshift-virtualization-qe-bot
Copy link
Copy Markdown

New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants