-
Notifications
You must be signed in to change notification settings - Fork 68
fix: Add missing arg in wait function which overwrites parent function
#2574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughTwo OCP resource classes (DataVolume and VirtualMachineImport) now accept a configurable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
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. |
|
/verified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/datavolume.py (1)
260-260: Optional: Explicit return None is unnecessary.The explicit
return Noneis functionally correct but unnecessary since Python functions returnNoneby default. While it does provide symmetry with theifbranch's explicit return, it can be omitted without changing behavior.Apply this diff if you prefer to omit the explicit return:
self.wait_for_status(status=self.Status.SUCCEEDED, timeout=timeout) self.pvc.wait_for_status(status=PersistentVolumeClaim.Status.BOUND, timeout=timeout) - return None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ocp_resources/datavolume.py(1 hunks)ocp_resources/virtual_machine_import.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ocp_resources/datavolume.py (2)
ocp_resources/resource.py (3)
wait(897-921)wait_for_status(958-1004)status(1062-1072)ocp_resources/persistent_volume_claim.py (1)
Status(12-13)
🔇 Additional comments (3)
ocp_resources/virtual_machine_import.py (2)
192-199: LGTM! Fixes the TypeError for sleep parameter.The addition of the
sleep=1parameter correctly resolves the issue where callers passingsleepas a keyword argument would encounter a TypeError. The default value maintains backward compatibility with existing calls.
201-208: LGTM! Sleep parameter properly threaded through.The
sleepparameter is correctly passed toTimeoutSampler, replacing the previous hardcoded value and enabling configurable polling intervals.ocp_resources/datavolume.py (1)
251-253: LGTM! Fixes TypeError and correctly threads sleep parameter.The addition of the
sleep=1parameter resolves the TypeError issue, and the parameter is correctly passed to the parent'swait()method in thewait_for_exists_onlypath.
wait function which overwrites parent functionwait function which overwrites parent function
|
/retest conventional-title |
|
/lgtm |
Short description:
Some resources overwrite
waitfunction but did not declare the parent function'ssleeparg in their signature.This led to:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit