Storage: Add windows OADP test#4603
Open
dalia-frank wants to merge 2 commits intoRedHatQE:mainfrom
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds OADP Velero fixtures and a tier3 Velero test to back up/restore Windows VMs without DataMover, introduces HTTP-backed DataVolume VM provisioning and Windows file write/verify utilities, and moves the Windows file verification helper into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
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
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4603 +/- ##
==========================================
+ Coverage 98.63% 98.66% +0.03%
==========================================
Files 25 25
Lines 2420 2476 +56
==========================================
+ Hits 2387 2443 +56
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot-5
requested review from
Ahmad-Hafe,
RoniKishner,
acinko-rh,
dshchedr,
ema-aka-young,
geetikakay,
josemacassan,
jpeimer and
kshvaika
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/data_protection/oadp/conftest.py`:
- Around line 178-195: The fixture currently uses latest_windows =
py_config.get("latest_windows_os_dict") or {} which masks missing config and
passes None into create_windows_vm_from_dv_template and wait_for_windows_vm;
change to require and validate the config: fetch latest_windows via
py_config["latest_windows_os_dict"] (or raise a clear exception if missing) and
assert that IMAGE_PATH_STR, DV_SIZE_STR, and OS_VERSION_STR keys are present and
non-empty in latest_windows, raising a ValueError/RuntimeError with a
descriptive message if any are missing before calling
create_windows_vm_from_dv_template and get_test_artifact_server_url so the test
fails fast on bad/missing metadata.
In `@tests/data_protection/oadp/test_velero.py`:
- Around line 101-130: The new Windows OADP test
(test_backup_and_restore_windows_vm) lacks the required STD/STP traceability:
add or update the module-level docstring to include an STP link (or if no STP
exists, an RFE/Jira-epic link) and create an STD placeholder test/marker before
the implemented test (e.g., a minimal test function named
test_std_windows_backup_restore or a module-level note) that contains the STD
docstring describing scope, preconditions and test steps for review; ensure the
STD placeholder is reviewed/approved before keeping the implemented test and
include the STP/RFE/Jira identifier in the implemented test docstring or as a
module-level comment to satisfy traceability rules.
In `@utilities/virt.py`:
- Around line 1740-1742: The current call builds a single shell string for
Get-Content which breaks on spaces/wildcards; change the command tokens so
PowerShell receives Get-Content with the -LiteralPath parameter and the path as
a separate token (use the existing variables cmd, file_name_with_path and
run_ssh_commands), e.g. construct cmd so it invokes "powershell -command
Get-Content -LiteralPath <path>" with the path passed as its own token rather
than interpolated into a quoted string; keep the call to run_ssh_commands and
the assertion unchanged.
🪄 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: b453c7f3-eafb-4ce7-9edf-155ca92ba735
📒 Files selected for processing (6)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pyutilities/virt.py
💤 Files with no reviewable changes (1)
- tests/storage/storage_migration/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:failed
dalia-frank
force-pushed
the
oadp-windows-backup-restore
branch
from
11b3496 to
89350eb
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/data_protection/oadp/conftest.py (1)
169-197:⚠️ Potential issue | 🟠 MajorFail fast on required Windows metadata.
latest_windows.get(...)and.get(..., {})still mask missingIMAGE_PATH_STR,DV_SIZE_STR,TEMPLATE_LABELS_STR, andOS_VERSION_STRvalues. That turns a config bug into an opaque VM/setup failure later. Use direct key access here so the fixture stops at the config boundary.Suggested fix
- image_url=f"{get_test_artifact_server_url()}{latest_windows.get(IMAGE_PATH_STR)}", - dv_size=latest_windows.get(DV_SIZE_STR), - template_labels=latest_windows.get(TEMPLATE_LABELS_STR, {}), + image_url=f"{get_test_artifact_server_url()}{latest_windows[IMAGE_PATH_STR]}", + dv_size=latest_windows[DV_SIZE_STR], + template_labels=latest_windows[TEMPLATE_LABELS_STR], ... - version=latest_windows.get(OS_VERSION_STR), + version=latest_windows[OS_VERSION_STR],As per coding guidelines, "No defensive programming - fail-fast, don't hide bugs with fake defaults."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data_protection/oadp/conftest.py` around lines 169 - 197, The fixture windows_vm_with_data_volume_template currently uses latest_windows.get(IMAGE_PATH_STR), .get(DV_SIZE_STR), .get(TEMPLATE_LABELS_STR, {}), and .get(OS_VERSION_STR) which hides missing config keys; change these to direct key access (e.g. latest_windows[IMAGE_PATH_STR], latest_windows[DV_SIZE_STR], latest_windows[TEMPLATE_LABELS_STR], latest_windows[OS_VERSION_STR]) so a KeyError is raised immediately at fixture creation instead of producing opaque failures later in create_windows_vm_from_dv_template, wait_for_windows_vm, or write_file_windows_vm_for_oadp; update all occurrences in the fixture accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/data_protection/oadp/conftest.py`:
- Around line 169-197: The fixture windows_vm_with_data_volume_template
currently uses latest_windows.get(IMAGE_PATH_STR), .get(DV_SIZE_STR),
.get(TEMPLATE_LABELS_STR, {}), and .get(OS_VERSION_STR) which hides missing
config keys; change these to direct key access (e.g.
latest_windows[IMAGE_PATH_STR], latest_windows[DV_SIZE_STR],
latest_windows[TEMPLATE_LABELS_STR], latest_windows[OS_VERSION_STR]) so a
KeyError is raised immediately at fixture creation instead of producing opaque
failures later in create_windows_vm_from_dv_template, wait_for_windows_vm, or
write_file_windows_vm_for_oadp; update all occurrences in the fixture
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12ba0557-57a1-45e6-84ca-049aa8eb3e33
📒 Files selected for processing (6)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pyutilities/virt.py
💤 Files with no reviewable changes (1)
- tests/storage/storage_migration/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
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
6 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.
Short description:
Add automated backup and restore tests for Windows.
More details:
Now that the foundational Windows bug (CNV-84547) has been resolved, we are enabling the test suite for Windows backup and restore. These tests will provide the initial baseline coverage and ensure this functionality remains stable moving forward.
What this PR does / why we need it:
Feature Validation: Implements tests for Windows backup and restore functionality, which was previously blocked.
Continuous Coverage: Ensures that Windows support is tested on a regular basis now that the environment is ready.
Which issue(s) this PR fixes:
Fixes: CNV-84548
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-84548
Assisted-by: cursor
Summary by CodeRabbit