MTV-5824: Add ca.crt provider secret field migration test#563
Open
AmenB wants to merge 1 commit into
Open
Conversation
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
PR Summary by QodoAdd tier1 cold migration test for provider secret Description
Diagram
High-Level Assessment
Files changed (6)
|
Contributor
WalkthroughAdds configurable CA certificate storage, a shared inventory factory, and a cold migration test scenario that expects the source secret to use Changesca.crt Secret Field and Cold Migration Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
AmenB
changed the title
Code Review by Qodo
Context used✅ Compliance rules (platform):
81 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
utilities/utils.py (1)
163-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLOW: Fail fast on invalid
ca_cert_keyvalues
ca_cert_keyis used directly as a Secret field key. If it is empty (or malformed), failure is deferred to Secret creation and the error becomes harder to diagnose. Validate early in_fetch_and_store_cacert().Suggested fix
def _fetch_and_store_cacert( source_provider_data: dict[str, Any], secret_string_data: dict[str, Any], tmp_dir: pytest.TempPathFactory | None, session_uuid: str, ca_cert_key: str = "cacert", ) -> Path: @@ + if not ca_cert_key: + raise ValueError("ca_cert_key must be a non-empty secret field name") + cert_file = generate_ca_cert_file( provider_fqdn=source_provider_data["fqdn"], cert_file=tmp_dir.mktemp(source_provider_type.upper()) / f"{source_provider_type}_{session_uuid}_cert.crt", ) secret_string_data[ca_cert_key] = cert_file.read_text()As per coding guidelines, “Code must never result in
NonewhenNoneis not valid. Fail early with clear errors.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utilities/utils.py` around lines 163 - 189, The ca_cert_key parameter in the _fetch_and_store_cacert() function is used directly as a dictionary key without validation, which can lead to cryptic errors during Secret creation if an empty or malformed value is passed. Add early validation at the start of the _fetch_and_store_cacert() function to check if ca_cert_key is a non-empty string, and raise a ValueError with a clear message if it is invalid. This ensures failures are caught immediately with diagnostic context rather than being deferred to downstream Secret creation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cold/conftest.py`:
- Around line 62-73: The code uses truthiness-based checks with `if not
ca_crt_source_provider.ocp_resource:` and `if not provider_class:` when checking
for None values. Replace these with explicit `is None` checks to follow the
coding guidelines and avoid unexpected behavior from custom `__bool__`
implementations. Change `if not ca_crt_source_provider.ocp_resource:` to `if
ca_crt_source_provider.ocp_resource is None:` and change `if not
provider_class:` to `if provider_class is None:`.
In `@tests/cold/test_ca_crt_migration.py`:
- Around line 134-144: The test_check_vms method declares target_namespace as a
fixture parameter but does not use it anywhere in the method body. Remove the
target_namespace parameter from the method signature of test_check_vms to comply
with the guideline that each test method should only request the fixtures it
actually needs, avoiding unnecessary fixture setup overhead.
- Around line 46-156: Add complete type annotations to all five test methods
(test_create_storagemap, test_create_networkmap, test_create_plan,
test_migrate_vms, and test_check_vms) in the test class. For each method, add
type hints to all parameters based on their fixture types and add the return
type annotation `-> None` since test methods do not return values. The
parameters should be annotated with their appropriate types from the fixture
definitions to ensure fixture API changes are caught at type-check time.
---
Outside diff comments:
In `@utilities/utils.py`:
- Around line 163-189: The ca_cert_key parameter in the
_fetch_and_store_cacert() function is used directly as a dictionary key without
validation, which can lead to cryptic errors during Secret creation if an empty
or malformed value is passed. Add early validation at the start of the
_fetch_and_store_cacert() function to check if ca_cert_key is a non-empty
string, and raise a ValueError with a clear message if it is invalid. This
ensures failures are caught immediately with diagnostic context rather than
being deferred to downstream Secret creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95055e79-867b-4c84-816a-79a532d48550
📒 Files selected for processing (6)
conftest.pypytest.initests/cold/conftest.pytests/cold/test_ca_crt_migration.pytests/tests_config/config.pyutilities/utils.py
|
Code review by qodo was updated up to the latest commit b04ff09 |
redhat-qe-bot1
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
|
Code review by qodo was updated up to the latest commit 5bb596d |
Contributor
Author
|
/verified |
myakove
reviewed
Jun 28, 2026
myakove
left a comment
myakove
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Found 1 issue(s) in this PR:
💡 Suggestions (1)
| File | Line | Issue |
|---|---|---|
utilities/utils.py |
278 | [SUGGESTION] create_source_provider has no docstring despite signature cha |
Review generated by pi
|
Code review by qodo was updated up to the latest commit 7ec4f26 |
|
Clean rebase detected — no code changes compared to previous head ( |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cold/test_ca_crt_migration.py`:
- Around line 57-129: The CA CRT migration test currently validates only the
downstream flow, not the provider Secret contents being generated. In
test_create_storagemap or test_create_networkmap, add a direct assertion on the
created provider Secret before proceeding, verifying it contains ca.crt and does
not contain cacert. Use the existing helpers and fixtures around
get_storage_migration_map/get_network_migration_map and the provider Secret
object so the key under test is checked explicitly rather than inferred from
StorageMap/Plan success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 031e7783-eba1-43f6-b71f-d5544b31c7c1
📒 Files selected for processing (6)
conftest.pylibs/forklift_inventory.pytests/cold/conftest.pytests/cold/test_ca_crt_migration.pytests/tests_config/config.pyutilities/utils.py
|
Code review by qodo was updated up to the latest commit 023a1df |
|
Code review by qodo was updated up to the latest commit e3f1d98 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/utils.py (1)
158-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHIGH: Reject unsupported
ca_cert_keyvalues up front.This patch only documents
"cacert"and"ca.crt", but the new API accepts any string and blindly writes it into the providerSecret. A typo here silently changes the field under test instead of failing fast.Possible fix
+_SUPPORTED_CA_CERT_KEYS = {"cacert", "ca.crt"} + def _fetch_and_store_cacert( source_provider_data: dict[str, Any], secret_string_data: dict[str, Any], tmp_dir: pytest.TempPathFactory | None, session_uuid: str, ca_cert_key: str = "cacert", ) -> Path: @@ + if ca_cert_key not in _SUPPORTED_CA_CERT_KEYS: + raise ValueError( + f"Unsupported ca_cert_key '{ca_cert_key}'. Expected one of {_SUPPORTED_CA_CERT_KEYS}." + ) + source_provider_type = source_provider_data["type"]As per coding guidelines, "Use
ValueErrorfor invalid input/config."Also applies to: 278-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utilities/utils.py` around lines 158 - 164, The _fetch_and_store_cacert helper now accepts ca_cert_key as free-form input, so add an upfront validation step in this function (and any related call site around the provider Secret write path) to allow only the documented values like "cacert" and "ca.crt". If an unsupported key is passed, raise ValueError immediately instead of silently writing a different field, so typos fail fast and the secret shape stays predictable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cold/test_ca_crt_migration.py`:
- Around line 58-63: The test_verify_ca_crt_secret method is requesting an extra
pytest fixture that is never used. Remove target_namespace from the
test_ca_crt_migration test signature so the fixture list matches the actual
dependencies and pytest does not resolve unnecessary setup.
---
Outside diff comments:
In `@utilities/utils.py`:
- Around line 158-164: The _fetch_and_store_cacert helper now accepts
ca_cert_key as free-form input, so add an upfront validation step in this
function (and any related call site around the provider Secret write path) to
allow only the documented values like "cacert" and "ca.crt". If an unsupported
key is passed, raise ValueError immediately instead of silently writing a
different field, so typos fail fast and the secret shape stays predictable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1d0fa21-66ed-46fb-8231-4fd2be373a2c
📒 Files selected for processing (6)
conftest.pylibs/forklift_inventory.pytests/cold/conftest.pytests/cold/test_ca_crt_migration.pytests/tests_config/config.pyutilities/utils.py
|
Code review by qodo was updated up to the latest commit 1c10f86 |
Contributor
Author
|
/verified |
|
Code review by qodo was updated up to the latest commit 2dfb2f3 |
Contributor
Author
|
/verified |
myakove
reviewed
Jun 29, 2026
myakove
left a comment
myakove
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Found 2 issue(s) in this PR:
💡 Suggestions (2)
| File | Line | Issue |
|---|---|---|
libs/forklift_inventory.py |
62 | type: ignore[call-arg] without explanation |
utilities/utils.py |
282 | [SUGGESTION] Docstring Args missing type annotations per project standard. |
Review generated by pi
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/verified |
|
Code review by qodo was updated up to the latest commit 461e73a |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cold/test_ca_crt_migration.py`:
- Around line 60-65: The test_verify_ca_crt_secret step is requesting an unused
prepared_plan fixture, which adds unnecessary pytest setup. Remove prepared_plan
from the test_verify_ca_crt_secret method signature and keep only the fixtures
actually used there, namely ca_crt_source_provider and ocp_admin_client, so the
incremental test step stays lean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0183cf5b-c200-4c3a-b29b-b06e350cf8fa
📒 Files selected for processing (6)
conftest.pylibs/forklift_inventory.pytests/cold/conftest.pytests/cold/test_ca_crt_migration.pytests/tests_config/config.pyutilities/utils.py
Comment on lines
+60
to
+65
| def test_verify_ca_crt_secret( | ||
| self, | ||
| prepared_plan: dict[str, Any], | ||
| ca_crt_source_provider: BaseProvider, | ||
| ocp_admin_client: "DynamicClient", | ||
| ) -> None: |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
LOW: Drop the unused prepared_plan fixture from this step.
Line 62 is never read, so pytest resolves extra setup for the first incremental step with no benefit.
Possible fix
def test_verify_ca_crt_secret(
self,
- prepared_plan: dict[str, Any],
ca_crt_source_provider: BaseProvider,
ocp_admin_client: "DynamicClient",
) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_verify_ca_crt_secret( | |
| self, | |
| prepared_plan: dict[str, Any], | |
| ca_crt_source_provider: BaseProvider, | |
| ocp_admin_client: "DynamicClient", | |
| ) -> None: | |
| def test_verify_ca_crt_secret( | |
| self, | |
| ca_crt_source_provider: BaseProvider, | |
| ocp_admin_client: "DynamicClient", | |
| ) -> None: |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 62-62: Unused method argument: prepared_plan
(ARG002)
[warning] 64-64: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cold/test_ca_crt_migration.py` around lines 60 - 65, The
test_verify_ca_crt_secret step is requesting an unused prepared_plan fixture,
which adds unnecessary pytest setup. Remove prepared_plan from the
test_verify_ca_crt_secret method signature and keep only the fixtures actually
used there, namely ca_crt_source_provider and ocp_admin_client, so the
incremental test step stays lean.
Source: Linters/SAST tools
myakove
reviewed
Jun 29, 2026
myakove
left a comment
myakove
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Found 1 issue(s) in this PR:
💡 Suggestions (1)
| File | Line | Issue |
|---|---|---|
tests/cold/conftest.py |
53 | [SUGGESTION] insecure receives None instead of bool when config key is |
Review generated by pi
| tmp_dir=tmp_path_factory, | ||
| ocp_admin_client=ocp_admin_client, | ||
| destination_ocp_secret=destination_ocp_secret, | ||
| insecure=get_value_from_py_config(value="source_provider_insecure_skip_verify"), |
Collaborator
There was a problem hiding this comment.
[SUGGESTION] insecure receives None instead of bool when config key is absent.
get_value_from_py_config(...) returns None when the key doesn't exist, but create_source_provider declares insecure: bool. Works because None is falsy, but type mismatch.
Suggested change
| insecure=get_value_from_py_config(value="source_provider_insecure_skip_verify"), | |
| insecure=bool(get_value_from_py_config(value="source_provider_insecure_skip_verify")), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ca.crtfield in provider secrets (MTV-4561 / MTV-4647 | Support standard Kubernetes ca.crt field in provider secrets kubev2v/forklift#5549)ca_cert_keyparameter to_fetch_and_store_cacert()andcreate_source_provider()— defaults to"cacert", no behavior change for existing testsmtv-tests-rhel8VM — no new infrastructure requiredTest plan
uv run pytest -s --collect-only -m tier1and verifyTestCaCrtColdMigrationis collectedca.crtkey (notcacert)TestSanityColdMtvMigrationto verify legacycacertpath is unaffectedsource_provider_insecure_skip_verify=true🤖 Generated with Claude Code
Summary by CodeRabbit
ca.crt) and to build the matching inventory.ca.crt(notcacert) and creates Storagemap, NetworkMap, and Plan resources.ca_cert_key), defaulting tocacert.