Skip to content

feat: add rdmAsLun parameter to Plan resource#2754

Open
krcmarik wants to merge 1 commit into
RedHatQE:mainfrom
krcmarik:rdmAsLun
Open

feat: add rdmAsLun parameter to Plan resource#2754
krcmarik wants to merge 1 commit into
RedHatQE:mainfrom
krcmarik:rdmAsLun

Conversation

@krcmarik

@krcmarik krcmarik commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Add rdm_as_lun parameter to the Plan class to support mapping RDM (Raw Device Mapping) disks as LUN devices with SCSI bus on the target VM during MTV migrations. Only applies to vSphere source providers.

Summary by CodeRabbit

  • New Features
    • Added support for an optional rdm_as_lun setting when creating a plan.
    • When enabled, this value is included in the generated plan configuration.
    • Documented that this option applies to vSphere-based source providers only.

Add rdm_as_lun parameter to the Plan class to support mapping RDM
(Raw Device Mapping) disks as LUN devices with SCSI bus on the target
VM during MTV migrations. Only applies to vSphere source providers.
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5397de51-e3f8-40b6-9fbc-a5fb5ed8bcc9

📥 Commits

Reviewing files that changed from the base of the PR and between 3d29c21 and 343f639.

📒 Files selected for processing (1)
  • ocp_resources/plan.py

Walkthrough

Adds an optional rdm_as_lun boolean parameter to the Plan resource class in ocp_resources/plan.py, applicable only to vSphere source providers, storing it on the instance and including it in the generated spec dictionary when set.

Changes

Plan rdm_as_lun support

Layer / File(s) Summary
rdm_as_lun parameter and spec generation
ocp_resources/plan.py
Documents rdm_as_lun in the class docstring, adds it as an optional constructor argument, stores it as an instance attribute, and conditionally emits spec["rdmAsLun"] in to_dict() when set.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Related Issues: None found in provided context.

Suggested labels: enhancement

Suggested reviewers: None specified.


🐰 A whisper of LUN, a plan takes shape,
vSphere's disks now find their escape,
One boolean flag, tucked in the spec,
A tiny hop, no need to fret. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is only one sentence and omits the required template sections like details, issue links, special notes, and bug info. Expand the description to fill the template sections: Short description, More details, Why needed, issue links, Special notes, and Bug.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding rdmAsLun support to Plan.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add rdmAsLun option to Plan resource for vSphere RDM migrations

✨ Enhancement 🕐 10-20 Minutes

Grey Divider

AI Description

• Add rdm_as_lun parameter to the Plan resource API.
• Serialize the option into the Plan spec as rdmAsLun when provided.
• Document behavior: map vSphere RDM disks as SCSI LUNs on the target VM.
Diagram

graph TD
  A["Caller code"] --> B["Plan class"] --> C["Plan.to_dict()"] --> D["Plan CR spec"] --> E["MTV/Forklift controller"]
Loading
High-Level Assessment

The PR’s approach (add an explicit optional parameter and map it to the CRD field name during serialization) is the most direct and consistent with how other Plan fields are modeled, while keeping API usage discoverable via type hints and docstrings.

Files changed (1) +9 / -0

Enhancement (1) +9 / -0
plan.pyAdd rdm_as_lun Plan option and serialize as rdmAsLun +9/-0

Add rdm_as_lun Plan option and serialize as rdmAsLun

• Introduces a new optional 'rdm_as_lun' constructor parameter on 'Plan' and stores it on the instance. Updates 'to_dict()' to include 'spec.rdmAsLun' when set, and extends the class docstring to document vSphere-only RDM-as-LUN behavior.

ocp_resources/plan.py

@redhat-qe-bot1

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: Disabled for this repository
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: All label categories are enabled (default configuration)

📋 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)
  • /regenerate-welcome - Regenerate this welcome message
  • /security-override - Set security check runs to pass (maintainers only)
  • /security-override cancel - Re-run security checks

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 python-module-install - Test Python package installation
  • /retest conventional-title - Validate commit message format
  • /retest all - Run all available tests

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3
  • /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

  • /rebase - Rebase this PR branch onto its base branch

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. Status Checks: All required status checks must pass
  3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove
  • rnetser

Reviewers:

  • myakove
  • rnetser
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)
Security Checks
  • Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
  • Committer Identity Check: Verifies last committer matches PR author
  • Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • 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.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Informational

1. vSphere-only flag not enforced 🐞 Bug ⚙ Maintainability
Description
Plan documents rdm_as_lun as “Only applies to vSphere source providers”, but to_dict() will
serialize spec["rdmAsLun"] whenever the parameter is set, regardless of source provider type. This
lets callers generate Plan specs that contradict the class’ documented constraint, making the API
contract ambiguous.
Code

ocp_resources/plan.py[R223-224]

+            if self.rdm_as_lun is not None:
+                spec["rdmAsLun"] = self.rdm_as_lun
Relevance

⭐ Low

Team often rejects adding spec validation/constraints in to_dict (e.g., required/non-empty checks
rejected in PRs #2529, #2520).

PR-#2529
PR-#2520
PR-#2545

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The docstring explicitly states the option is vSphere-only, but the serializer adds rdmAsLun with
no conditional logic tied to source provider type.

ocp_resources/plan.py[45-48]
ocp_resources/plan.py[223-224]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Plan` claims `rdm_as_lun` only applies to vSphere sources, but the implementation always emits `spec.rdmAsLun` when set. This creates an ambiguous contract: the class documentation implies a constraint that the code does not enforce.

### Issue Context
The class currently does not track the source provider type (only the provider name/namespace), so it cannot enforce the constraint unless additional context is supplied.

### Fix Focus Areas
- ocp_resources/plan.py[45-48]
- ocp_resources/plan.py[223-224]

### Suggested fix approaches (pick one)
1) **Enforce the constraint**: add an optional `source_provider_type` (or similar) parameter to `Plan.__init__`, and if `rdm_as_lun is not None` and `source_provider_type != ProviderType.VSPHERE`, raise `ValueError`.
2) **Clarify documentation**: if enforcement is intentionally out-of-scope for this class, adjust the docstring wording to indicate that the field is *only meaningful/used by MTV* for vSphere sources, but will still be serialized if provided.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

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.

2 participants