MTV-3129: Add ClusterRole (forklift-migrator-role) migration tests for local remote cluster#293
MTV-3129: Add ClusterRole (forklift-migrator-role) migration tests for local remote cluster#293AmenB wants to merge 1 commit into
Conversation
|
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:
WalkthroughAdds a ClusterRole-based destination provider fixture, three end-to-end MTV clusterrole migration tests (cold, warm, configmap/secret), supporting test configs, new migration utilities for plan execution and verification, a VIRTCTL_PATH env override, and constant FORKLIFT_MIGRATOR_ROLE_NAME. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Fixture as clusterrole_destination_ocp_provider
participant OCP as OpenShift Cluster
participant Forklift as Forklift Provider
participant Migration as Migration Plan
Test->>Fixture: request provider
Fixture->>OCP: create ServiceAccount
OCP-->>Fixture: SA created
Fixture->>OCP: create ClusterRoleBinding (SA -> forklift-migrator-role)
OCP-->>Fixture: CRB created
Fixture->>OCP: create service-account-token Secret
OCP-->>Fixture: Secret created (base64 token)
Fixture->>Fixture: decode token (base64)
Fixture->>OCP: create provider Secret (token + insecureSkipVerify)
OCP-->>Fixture: provider Secret created
Fixture->>Forklift: create Provider CR referencing provider Secret
Forklift-->>Fixture: Provider CR created
Fixture-->>Test: yield OCPProvider
Test->>Migration: run_clusterrole_migration(...)
Migration->>OCP: create/execute Plan, migrate VMs/resources
OCP-->>Migration: migration finished
Test->>Test: verify_vms_running / verify_configmap_migrated / verify_secret_migrated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions ✨Latest suggestions up to 383e62f
Previous suggestionsSuggestions up to commit 41829a4
Suggestions up to commit b8d4e97
✅ Suggestions up to commit b9caf3c
✅ Suggestions up to commit 7587cdd
✅ Suggestions up to commit fe3c198
✅ Suggestions up to commit d127edb
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 795-802: The local variables service_account and token_secret are
assigned from create_and_store_resource but never used; either drop the
assignments or rename them to _service_account and _token_secret to indicate
intentional discard. Update the calls to create_and_store_resource (the
invocations that currently assign to service_account and token_secret) so they
do not create unused locals, ensuring fixture_store still receives/stores the
resources as before; apply the same change to the other occurrences around the
create_and_store_resource calls referenced in the review.
- Around line 864-867: The fixture currently yields an OCPProvider instance
(yield OCPProvider(ocp_resource=provider, fixture_store=fixture_store)) but has
no teardown logic; replace the yield with a return to follow Ruff PT022 and make
the intent explicit that this fixture has no local teardown (i.e., change to
return OCPProvider(...)). Keep the same arguments (ocp_resource=provider,
fixture_store=fixture_store) and rely on fixture_store for session-level
cleanup.
- Around line 461-463: Replace the double call to os.environ.get("VIRTCTL_PATH")
with a single lookup stored in a local variable (e.g., virtctl =
os.environ.get("VIRTCTL_PATH")), then check that variable (if virtctl:) and
return Path(virtctl); modify the block containing the current os.environ.get
usage so only one environment lookup is performed and Path() is invoked with the
stored value.
- Around line 773-779: Add full type annotations to the fixture function
clusterrole_destination_ocp_provider: annotate its return type as
Generator[OCPProvider, None, None] and annotate each parameter (fixture_store,
ocp_admin_client, session_uuid, target_namespace) with their appropriate types
(e.g., fixture_store: dict, ocp_admin_client: OCPClient or the actual client
type used in the repo, session_uuid: str, target_namespace: str) so the
signature uses built-in typing and matches the guideline (Generator is already
imported).
- Around line 828-831: Currently `_has_token` re-instantiates Secret on every
poll; instead create the Secret wrapper once (Secret(client=ocp_admin_client,
name=token_secret_name, namespace=target_namespace)) in the outer scope and have
`_has_token` only call the existing instance's refresh/wait and read its data
(e.g., call s.wait() or s.reload()/s.refresh() on that single `s` and return
(s.instance.data or {}).get("token")). Update references to `_has_token`,
`Secret`, `s.wait()`, and `s.instance.data` accordingly so the wrapper isn't
recreated each poll.
- Around line 827-840: The TimeoutSampler loop using _has_token should catch
TimeoutExpiredError instead of relying on a for-else (which is unreachable);
wrap the for sample in a try/except that imports TimeoutExpiredError, on success
set token_b64 = sample as now, and in the except TimeoutExpiredError raise a
ValueError that includes token_secret_name and sa_name to preserve the
descriptive error path.
In `@tests/test_mtv_clusterrole_migration.py`:
- Around line 198-232: test_mtv_clusterrole_configmap_secret_migration passes
source_vms_namespace (which can be None for non-OpenShift sources) into
create_and_store_resource causing failures; add an early guard at the start of
test_mtv_clusterrole_configmap_secret_migration to assert or skip when
source_vms_namespace is None (e.g., pytest.skip("source_vms_namespace is None:
test requires an OpenShift source") or raise a clear ValueError) before calling
create_and_store_resource for ConfigMap and Secret, referencing the function
name create_and_store_resource and the test function
test_mtv_clusterrole_configmap_secret_migration so the intent and location are
obvious.
- Around line 34-97: Extract the duplicated migration flow and VM verification
into shared helpers and call them from test_mtv_clusterrole_cold_migration (and
the other two tests); specifically, create a utility (e.g.,
utilities/clusterrole_utils.py) with a run_clusterrole_migration(...) function
that encapsulates the sequence using get_storage_migration_map,
get_network_migration_map, populate_vm_ids, create_plan_resource, and
execute_migration, and a verify_vms_running(...) function that iterates
prepared_plan["virtual_machines"] and asserts each
VirtualMachine.instance.status.printableStatus == VirtualMachine.Status.RUNNING;
then replace the duplicated blocks in test_mtv_clusterrole_cold_migration, the
warm test, and the configmap/secret test to call these helpers.
- Around line 116-169: The test_mtv_clusterrole_warm_migration test calls
execute_migration(...) without the cut_over parameter, so update the
execute_migration call to include cut_over=get_cutover_value() (matching other
warm migration tests) to ensure the warm migration completes its cutover phase;
locate the execute_migration invocation in test_mtv_clusterrole_warm_migration
and add the cut_over argument accordingly.
In `@utilities/clusterrole_utils.py`:
- Around line 42-43: The source/target existence checks in
verify_configmap_migrated (the `if not source_cm.exists: raise
AssertionError(...)` block) and verify_secret_migrated use mixed styles; make
them consistent by replacing explicit `raise AssertionError(...)` with an
`assert ... , msg` (or conversely replace the `assert` with `raise
AssertionError(msg)`) across both functions, update the message text to match
the existing assertions, and if you intentionally want different semantics add a
brief inline comment above the chosen form explaining why that form
(precondition vs test assertion) is required.
- Around line 60-103: The docstring for verify_secret_migrated is misleading: it
claims it "compares decoded data" but the implementation compares
Secret.instance.data (base64-encoded) values; either update the docstring to say
it compares raw/encoded Secret data or change the code to decode both source and
target data before comparison; locate the function verify_secret_migrated and
the use of Secret.instance.data and adjust accordingly (and optionally simplify
the per-key loop to a single equality check like source_data == target_data if
you prefer consistency with the ConfigMap helper).
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 461-470: conftest.py contains leftover git merge conflict markers
around the virtctl_binary logic which makes the module unparseable; remove the
conflict markers (<<<<<<<, =======, >>>>>>>) and keep the refactored
walrus-operator variant: in the virtctl_binary function use the assignment
expression (virtctl_env_path := os.environ.get("VIRTCTL_PATH")) and return
Path(virtctl_env_path) when truthy, ensuring the surrounding comment is tidy and
only one branch remains.
In `@tests/test_mtv_clusterrole_migration.py`:
- Around line 65-77: This file contains leftover git merge conflict markers
around three blocks that duplicate inline VM-check loops; remove the conflict
markers and the HEAD-side inline loops so the refactored version that calls the
helper verify_vms_running remains. Specifically, delete the <<<<<<< / ======= /
>>>>>>> regions and the duplicated for vm_config ... VirtualMachine(...)
assertions (the ones referencing prepared_plan, vm_config, VirtualMachine and
checking vm.instance.status.printableStatus) in all three locations and ensure
each test simply calls verify_vms_running(...) as intended.
- Around line 174-177: Replace the in-test runtime call to pytest.skip() for
source_vms_namespace with a collection-time skip marker: add an
`@pytest.mark.skipif` on the test (or enclosing test class) that checks
py_config.get("source_provider_type") != "openshift" and includes a clear reason
string, so the guard is evaluated at collection rather than inside the test
body; if py_config cannot be relied on at collection time, document the
exception near the test instead of calling pytest.skip() at runtime. Ensure you
update the test that currently references source_vms_namespace so the skip
condition is applied via the decorator rather than inside the function body.
In `@utilities/clusterrole_utils.py`:
- Around line 31-45: Add full docstrings to the functions missing them: update
run_clusterrole_migration and verify_vms_running to include an initial summary
plus explicit Args, Returns, and Raises sections; for run_clusterrole_migration
document parameters (ocp_admin_client, fixture_store, source_provider,
destination_provider, prepared_plan, source_provider_inventory,
target_namespace, multus_network_name, cut_over), the return type (None), and
any exceptions that may be raised (e.g., migration/plan creation errors or wait
timeouts), and do the same for verify_vms_running documenting its parameters,
return value, and possible exceptions so both functions comply with the
project's docstring guidelines.
- Around line 131-138: This file contains unresolved git conflict markers that
break Python parsing; remove all conflict markers (<<<<<<<, =======, >>>>>>>) in
the three regions and keep the refactor/assert-style branches instead of the
explicit raise branches: replace the source_cm presence check with the assert
form (assert source_cm.exists, f"Source ConfigMap {configmap_name} not found in
namespace {source_namespace}"), likewise choose the assert-style lines for the
other two conflict regions (the checks referencing the same ConfigMap/secret
existence variables used later in this module), then run a quick import or lint
to ensure there are no remaining markers and the module parses cleanly.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 834-836: The inner helper _has_token currently lacks a return type
annotation; update its signature to include the appropriate type (e.g., ->
Optional[str]) so the function reads as def _has_token() -> Optional[str]: and
ensure typing.Optional (or Optional) is imported if not already present; keep
the body unchanged (token_secret_ref.wait() and return
(token_secret_ref.instance.data or {}).get("token")) so static checkers like
Ruff ANN202 are satisfied.
- Around line 774-790: The fixture clusterrole_destination_ocp_provider is
missing formal docstring sections; update its docstring to include clearly
labeled Args (describing fixture_store, ocp_admin_client, session_uuid,
target_namespace and their types/roles), Returns (describe it returns an
OCPProvider), and Raises (list possible exceptions/errors the fixture may raise,
e.g., errors creating SA/token or missing ClusterRole), keeping the existing
narrative summary and ensuring wording matches project docstring style
guidelines.
- Around line 461-462: Remove the duplicate comment line in conftest.py (the
repeated "# if env variable VIRTCTL_PATH is set, use it.") so only a single
instance of that comment remains; locate the duplicated lines around the
VIRTCTL_PATH comment block and delete one of them to avoid redundancy.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified |
6 participants
PR Type
Tests, Enhancement
Description
Add comprehensive ClusterRole migration tests for forklift-migrator-role
Implement clusterrole_destination_ocp_provider fixture for token-based authentication
Add utility functions for ConfigMap and Secret migration verification
Fix virtctl_binary fixture to respect VIRTCTL_PATH environment variable
Add test configuration parameters for three ClusterRole migration scenarios
Diagram Walkthrough
File Walkthrough
conftest.py
Add token-based OCP provider fixture and env var supportconftest.py
base64import for token decodingClusterRoleBindingandServiceAccountresourcesvirtctl_binaryfixture to checkVIRTCTL_PATHenvironmentvariable first
clusterrole_destination_ocp_providersession-scopedfixture that creates ServiceAccount, binds to forklift-migrator-role,
generates token, and creates Forklift Provider CR
FORKLIFT_MIGRATOR_ROLE_NAMEconstant for the existingClusterRole
clusterrole_utils.py
Add ConfigMap and Secret migration verification utilitiesutilities/clusterrole_utils.py
verify_configmap_migratedfunction to assert ConfigMapexists in target namespace and data matches source
verify_secret_migratedfunction to assert Secret exists intarget namespace and data matches source
error messages
test_mtv_clusterrole_migration.py
Add ClusterRole migration test suitetests/test_mtv_clusterrole_migration.py
TestClusterroleColdMtvMigration: verifies cold migration withtoken-based provider
TestClusterroleWarmMtvMigration: verifies warm migration withtoken-based provider
TestClusterroleConfigmapSecretMigration: verifies ConfigMap and Secretmigration alongside VM migration
clusterrole_destination_ocp_providerfixture and verifyVM running status post-migration
config.py
Add test parameters for ClusterRole migration teststests/tests_config/config.py
test_mtv_clusterrole_cold_migrationconfiguration with singleRHEL8 VM and cold migration
test_mtv_clusterrole_warm_migrationconfiguration with singleRHEL8 VM powered on and warm migration enabled
test_mtv_clusterrole_configmap_secret_migrationconfigurationwith single RHEL8 VM and cold migration
Summary by CodeRabbit
New Features
Utilities
Tests