feat(storage): add deprecation warning to dv.wait()#2613
feat(storage): add deprecation warning to dv.wait()#2613myakove merged 4 commits intoRedHatQE:mainfrom
Conversation
WalkthroughA DeprecationWarning is emitted at the start of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/ocp_resources/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-13T19:32:11.177ZApplied to files:
🔇 Additional comments (1)
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/datavolume.py (1)
253-253: Consider using positional argument for consistency.For consistency with the existing deprecation warning at line 197, consider passing the message as a positional argument rather than a keyword argument.
warn( - message="DataVolume.wait() is deprecated and will be removed in the next version. Use wait_for_dv_success() instead.", + "DataVolume.wait() is deprecated and will be removed in the next version. Use wait_for_dv_success() instead.", category=DeprecationWarning, stacklevel=2, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go1.25.4.linux-amd64.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
ocp_resources/datavolume.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-python-wrapper PR: 2574
File: ocp_resources/datavolume.py:254-260
Timestamp: 2025-11-13T19:32:11.177Z
Learning: In `ocp_resources/datavolume.py`, the `DataVolume.wait()` method overrides the parent `wait()` method and accepts a `sleep` parameter. This parameter is only used when `wait_for_exists_only=True` to pass to `super().wait()`. The else branch intentionally does not use the `sleep` parameter as it has its own wait logic via `wait_for_status()` calls.
📚 Learning: 2025-11-13T19:32:11.177Z
Learnt from: rnetser
Repo: RedHatQE/openshift-python-wrapper PR: 2574
File: ocp_resources/datavolume.py:254-260
Timestamp: 2025-11-13T19:32:11.177Z
Learning: In `ocp_resources/datavolume.py`, the `DataVolume.wait()` method overrides the parent `wait()` method and accepts a `sleep` parameter. This parameter is only used when `wait_for_exists_only=True` to pass to `super().wait()`. The else branch intentionally does not use the `sleep` parameter as it has its own wait logic via `wait_for_status()` calls.
Applied to files:
ocp_resources/datavolume.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ocp_resources/datavolume.py (1)
252-257: Past review comment has been addressed.The deprecation warning now includes both the grammar fix ("in the next version") and the recommended alternative method (
wait_for_dv_success()). The implementation is correct and the warning will properly guide users to the replacement API.Optional: Consider style consistency with the existing deprecation warning.
For consistency with the deprecation warning at line 196-200, consider using positional arguments instead of keyword arguments:
def wait(self, timeout=TIMEOUT_10MINUTES, failure_timeout=TIMEOUT_2MINUTES, wait_for_exists_only=False, sleep=1): warn( - message="DataVolume.wait() is deprecated and will be removed in " - "the next version. Use wait_for_dv_success() instead.", - category=DeprecationWarning, + "DataVolume.wait() is deprecated and will be removed in " + "the next version. Use wait_for_dv_success() instead.", + DeprecationWarning, stacklevel=2, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/datavolume.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-python-wrapper PR: 2574
File: ocp_resources/datavolume.py:254-260
Timestamp: 2025-11-13T19:32:11.177Z
Learning: In `ocp_resources/datavolume.py`, the `DataVolume.wait()` method overrides the parent `wait()` method and accepts a `sleep` parameter. This parameter is only used when `wait_for_exists_only=True` to pass to `super().wait()`. The else branch intentionally does not use the `sleep` parameter as it has its own wait logic via `wait_for_status()` calls.
📚 Learning: 2025-11-13T19:32:11.177Z
Learnt from: rnetser
Repo: RedHatQE/openshift-python-wrapper PR: 2574
File: ocp_resources/datavolume.py:254-260
Timestamp: 2025-11-13T19:32:11.177Z
Learning: In `ocp_resources/datavolume.py`, the `DataVolume.wait()` method overrides the parent `wait()` method and accepts a `sleep` parameter. This parameter is only used when `wait_for_exists_only=True` to pass to `super().wait()`. The else branch intentionally does not use the `sleep` parameter as it has its own wait logic via `wait_for_status()` calls.
Applied to files:
ocp_resources/datavolume.py
|
/lgtm |
|
@dalia-frank please rebase and verified, I want to make a release and want to include this as well. |
|
/verified |
rnetser
left a comment
There was a problem hiding this comment.
please remove the go files
|
/verified |
|
/lgtm |
|
/lgtm |
|
/lgtm |
|
/verified |
|
/approve |
Short description:
Deprecate DataVolume.wait() in favor of wait_for_dv_success() to fix bad practice and prevent bugs.
More details:
DataVolume.wait() mixes two responsibilities—checking for existence and waiting for success—which breaks the pattern established by Resource.wait() and can lead to unexpected behavior in tests or automation. wait_for_dv_success() should be used when the DV needs to reach a successful state, as it clearly communicates intent and avoids these issues.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.