Skip to content

fix: OVA Windows guest detection when inventory osType is missing - #686

Merged
myakove merged 2 commits into
mainfrom
fix/issue-685-ova-windows-detection
Sep 23, 2026
Merged

myakove merged 2 commits into
mainfrom
fix/issue-685-ova-windows-detection

Conversation

@myakove

@myakove myakove commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #685.

Use the VM name as a fallback for OVA Windows guest detection only when osType is empty or missing. Clarify the project guidance in AGENTS.md.

Validation

  • pre-commit run --all-files passed.
  • Pytest was not run because project AGENTS.md prohibits AI from running tests that require live infrastructure and credentials.

Assisted-by: PI (gpt-6-luna)

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows guest detection for virtual machines when operating system metadata is missing, using the VM name as a fallback.

Use the VM name when OVA metadata omits the OS type, and document the product-only test rule.

Assisted-by: PI (gpt-6-luna) <noreply@pi.dev>
Signed-off-by: Meni Yakove <myakove@gmail.com>
@redhat-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: 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 (PR author or approvers)
  • /hold cancel - Unblock PR merging (PR author or approvers)
  • /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 build-container - Rebuild and test container image
  • /retest conventional-title - Validate commit message format
  • /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
  • /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:

  • krcmarik
  • myakove
  • solenoci

Reviewers:

  • krcmarik
  • myakove
  • solenoci
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge
AI Features
  • Conventional Title: Mode: fix (cursor/cursor-grok-4.6-high)
  • Cherry-Pick Conflict Resolution: Enabled (cursor/cursor-grok-4.6-high)
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
  • 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.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix OVA Windows detection when inventory OS type is missing

🐞 Bug fix 📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Detect Windows OVA guests from VM names when inventory omits osType.
• Preserve inventory metadata as authoritative whenever osType is present.
• Clarify live-infrastructure, product-only testing and test placement requirements.
Diagram

graph TD
  A["OVA VM request"] --> B["Forklift inventory"] --> C{"OS type present?"}
  C -- "Present" --> D["Metadata detection"] --> F["Windows flag"]
  C -- "Missing" --> E["Name fallback"] --> F
Loading
High-Level Assessment

The localized fallback is appropriate because inventory metadata remains authoritative and name inference is restricted to missing metadata. Extracting a shared detector or changing inventory parsing would add unnecessary scope for a provider-specific edge case.

Files changed (2) +11 / -3

Bug fix (1) +7 / -2
ova.pyFallback to VM names for OVA Windows detection +7/-2

Fallback to VM names for OVA Windows detection

• Normalizes missing or null inventory 'osType' values and preserves non-empty metadata as authoritative. When metadata is unavailable, a case-insensitive bounded regex recognizes Windows-oriented VM names without matching embedded substrings.

libs/providers/ova.py

Documentation (1) +4 / -1
AGENTS.mdClarify product-only integration test requirements +4/-1

Clarify product-only integration test requirements

• Documents that tests must exercise MTV through supported live integration or end-to-end paths. It prohibits mocked or internal-helper unit tests and reiterates feature-directory placement and execution restrictions.

AGENTS.md

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository: RedHatQE/mtv-api-tests/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: bf552938-4dd4-4410-9a29-38af0fdd4fed

📥 Commits

Reviewing files that changed from the base of the PR and between 5740ff4 and 753e27f.

📒 Files selected for processing (2)
  • AGENTS.md
  • libs/providers/ova.py
 ________________________
< GLaDOS? I can fix her! >
 ------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

qodo-code-review Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Bad inventory data misclassifies VMs ✓ Resolved 📘 Rule violation ⛨ Security
Description
vm_dict uses vm_data.get("osType") or "", collapsing non-string falsy provider values into the
missing-value case without validating their type. When inventory returns malformed values such as
False, 0, or an empty container, the value reaches the VM-name fallback and may classify the
guest as Windows instead of rejecting the invalid response.
Code

libs/providers/ova.py[45]

+            raw_os_type: str = vm_data.get("osType") or ""
Relevance

●●● Strong

Recent provider precedents favor validating malformed external metadata before fallback logic,
especially to prevent VM misclassification.

PR-#566

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 1263556 requires values obtained through .get() from provider-controlled responses
to be validated before they influence application logic. ForkliftInventory.get_vm() returns the
inventory API response directly, while the changed OVA logic converts every falsy osType to an
empty string and uses that result to select name-based detection without validating the original
value.

Rule 1263556: Validate values retrieved via .get\(\) from external or untrusted data sources
libs/providers/ova.py[45-49]
libs/forklift_inventory.py[116-119]

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

## Issue description
The OVA provider treats every falsy `osType` value as missing, allowing malformed provider data to influence Windows detection without type validation.

## Fix Focus Areas
- libs/providers/ova.py[45-49]

## Recommended Fix
Read `osType` into an untyped intermediate value, accept only `None` or `str`, and raise a descriptive `ValueError` for other types. Preserve the VM-name fallback only when the validated value is `None` or an empty string.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 100 rules
✅ Cross-repo context — repo relationships
Review mode: ⚖️ Balanced: This is a small, localized runtime change, but it adds input validation to OVA guest-detection logic and can affect behavior for malformed inventory data, warranting a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread libs/providers/ova.py Resolved
Reject non-string osType values from Qodo review.

Assisted-by: PI (gpt-6-luna) <noreply@pi.dev>
Signed-off-by: Meni Yakove <myakove@gmail.com>
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 6311bca

@myakove myakove changed the title Fix OVA Windows guest detection when inventory osType is missing fix: OVA Windows guest detection when inventory osType is missing Sep 23, 2026
@myakove

myakove commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

/verified

@myakove
myakove merged commit 25e763d into main Sep 23, 2026
8 of 9 checks passed
@myakove
myakove deleted the fix/issue-685-ova-windows-detection branch September 23, 2026 14:40
@redhat-qe-bot

Copy link
Copy Markdown

New container for ghcr.io/redhatqe/mtv-api-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.

OVA cold migration treats Windows guests as Linux for SSH validation

2 participants