Skip to content

Conversation

@dalia-frank
Copy link
Contributor

@dalia-frank dalia-frank commented Dec 17, 2025

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

  • Chores
    • Added a deprecation warning to the wait method. Users will see a DeprecationWarning when invoking it, while existing behavior and compatibility are preserved.
    • No changes to public APIs or signatures; current workflows continue to operate as before.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 17, 2025

Walkthrough

A DeprecationWarning is emitted at the start of DataVolume.wait() when invoked; the warning is issued before executing the existing control flow and no public signatures or other logic branches were changed.

Changes

Cohort / File(s) Change Summary
DataVolume deprecation warning
ocp_resources/datavolume.py
Emit a DeprecationWarning at the start of the wait() method before continuing with the existing wait logic (no signature or control-flow changes beyond the warning).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify the DeprecationWarning message and category are appropriate and consistent with project conventions.
  • Confirm the warning is emitted whenever wait() is entered and does not modify subsequent behavior or return values.

Suggested labels

can-be-merged, approved-myakove, lgtm-myakove

Suggested reviewers

  • myakove
  • rnetser

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the short description and rationale but leaves critical required template sections empty: 'What this PR does / why we need it', 'Which issue(s) this PR fixes', 'Special notes for reviewer', and 'Bug' are all blank. Complete the remaining template sections, particularly 'What this PR does / why we need it' with implementation details and identify any related issues in 'Which issue(s) this PR fixes'.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a deprecation warning to the DataVolume.wait() method, which aligns with the PR's primary objective.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28713d8 and fe722c2.

📒 Files selected for processing (1)
  • ocp_resources/datavolume.py
🧰 Additional context used
📓 Path-based instructions (1)
**/ocp_resources/**

⚙️ CodeRabbit configuration file

**/ocp_resources/**: SKIP reviewing any code between the markers:

  • Start: # Generated using https://github.com/RedHatQE/openshift-python-wrapper/blob/main/class_generator/README.md
  • End: # End of generated code

This is auto-generated code from the class-generator tool.
Only review code OUTSIDE these markers.
If someone modifies generated code directly, flag it as a violation -
changes should be made to the class-generator instead.

Files:

  • ocp_resources/datavolume.py
🧠 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
🔇 Additional comments (1)
ocp_resources/datavolume.py (1)

252-257: LGTM! Deprecation warning properly implemented.

The deprecation warning is correctly positioned and addresses all previous feedback:

  • Proper grammar: "in the next version"
  • Clear guidance to the replacement API: wait_for_dv_success()
  • Appropriate stacklevel=2 for accurate stack trace attribution
  • Emitted before any logic executes

The implementation achieves the PR objective of steering users away from the mixed-responsibility wait() method toward the more focused wait_for_dv_success() alternative.


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 and usage tips.

@redhat-qe-bot
Copy link
Contributor

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: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified

📋 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)

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

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

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. LGTM Count: Minimum 0 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No WIP, hold, or conflict labels
  5. Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

  • myakove
  • rnetser

Reviewers:

  • myakove
  • rnetser
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is automatically removed on each new commit
  • 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.

Copy link

@coderabbitai coderabbitai bot left a 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)

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3d55201 and 13c0cb1.

⛔ Files ignored due to path filters (1)
  • go1.25.4.linux-amd64.tar.gz is 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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 13c0cb1 and 28713d8.

📒 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

@dalia-frank dalia-frank changed the title Storage: add deprecation warning to dv.wait() feat(storage): add deprecation warning to dv.wait() Dec 18, 2025
@myakove
Copy link
Collaborator

myakove commented Dec 18, 2025

/lgtm
/approve

@myakove
Copy link
Collaborator

myakove commented Dec 18, 2025

@dalia-frank please rebase and verified, I want to make a release and want to include this as well.

@dalia-frank
Copy link
Contributor Author

/verified

Copy link
Collaborator

@rnetser rnetser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the go files

auto-merge was automatically disabled December 22, 2025 16:58

Head branch was pushed to by a user without write access

@dalia-frank
Copy link
Contributor Author

/verified

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.

7 participants