feat: added logic to stop waiting early to wait_for_condition method

[kshvaika]

Short description:

Previously, wait_for_condition continued waiting for the expected condition until the full timeout expired, even if the resource had already reached an invalid state.
This PR adds logic to stop waiting early and fail immediately when an unexpected condition is detected.

More details:

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewer:

Bug:

Summary by CodeRabbit

• New Features
  • Added ability to specify stop conditions when monitoring resource states so waits can fail early if a specified state is observed.
  • Introduced a new exception type to clearly surface unexpected resource condition states, enabling callers to handle these early-failure cases explicitly.

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

[coderabbitai]

Walkthrough

Adds a new public exception class ConditionError and extends Resource.wait_for_condition with stop_condition and stop_status parameters plus runtime logic that raises ConditionError when a matching stop condition/status is observed.

Changes

Cohort / File(s)	Summary
New exception type ocp_resources/exceptions.py	Added public ConditionError exception: class ConditionError(Exception): with docstring "Raised when a resource condition reaches an unexpected state." and empty body.
Resource condition monitoring enhancement ocp_resources/resource.py	Imported ConditionError and updated Resource.wait_for_condition signature to add `stop_condition: str

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Small, localized API addition and one new exception.
• Review focus:
  • wait_for_condition signature and typing for stop_condition / stop_status.
  • Correct comparison logic that detects the stop condition/status and raises ConditionError.
  • Import/export (public API) changes and docstring accuracy.

Possibly related PRs

• fix: ocp_resources, resource: Extend 'wait_for_condition' criteria #2544 — Modifies Resource.wait_for_condition signature and condition-evaluation/control-flow, closely related.

Suggested reviewers

• rnetser
• myakove

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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 (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding logic to enable early stopping in the wait_for_condition method.
Description check	✅ Passed	The PR description provides a clear short description and addresses the main purpose. However, several template sections (More details, Issue fixes, Special notes, Bug) are empty or minimal.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[kshvaika]

/build-and-push-container --build-arg OPENSHIFT_PYTHON_WRAPPER_COMMIT=3cc43800b87865784afd742c58ea712e41747aea

[rh-bot-1]

No build-and-push-container configured for this repository

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

ocp_resources/resource.py (2)

1256-1261: Clarify the ConditionError docstring.

The description "If the desired condition is not met and stop_condition is detected before timeout" is slightly misleading. The error is raised simply when the stop_condition is detected, regardless of the desired condition's state.

Consider rephrasing line 1261 to:

-            ConditionError: If the desired condition is not met and stop_condition is detected before timeout.
+            ConditionError: If stop_condition is detected with the specified stop_status before timeout.

---

1276-1280: LGTM! Early-stop logic correctly implemented.

The fail-fast check is properly placed before the desired condition check, and the error message includes the full condition details as suggested in previous review feedback.

Minor nitpick: The nested if statements could be combined for slightly cleaner code:

-                    if stop_condition:
-                        if actual_condition == {"type": stop_condition, "status": stop_status}:
-                            raise ConditionError(
-                                f"{self.kind} {self.name} reached stop_condition '{stop_condition}' in status '{stop_status}':\n{cond}"
-                            )
+                    if stop_condition and actual_condition == {"type": stop_condition, "status": stop_status}:
+                        raise ConditionError(
+                            f"{self.kind} {self.name} reached stop_condition '{stop_condition}' in status '{stop_status}':\n{cond}"
+                        )

📜 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 3cc4380 and 862969a.

📒 Files selected for processing (1)

• ocp_resources/resource.py (4 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2024-10-08T23:43:22.342Z

Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-08T23:43:22.342Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.

Applied to files:

• ocp_resources/resource.py

📚 Learning: 2025-08-11T16:42:27.596Z

Learnt from: servolkov
Repo: RedHatQE/openshift-python-wrapper PR: 2490
File: ocp_resources/route_advertisements.py:5-5
Timestamp: 2025-08-11T16:42:27.596Z
Learning: In the openshift-python-wrapper project, MissingRequiredArgumentError should be imported from ocp_resources.resource (not ocp_resources.exceptions), as it's re-exported there and this is the established pattern used throughout the codebase.

Applied to files:

• ocp_resources/resource.py

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/exceptions.py (1)

• ConditionError (93-96)

🪛 Ruff (0.14.5)

ocp_resources/resource.py

1278-1280: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (2)

ocp_resources/resource.py (2)

44-44: LGTM!

The ConditionError import is correctly placed and necessary for the new early-stop functionality.

---

1243-1244: LGTM!

The new parameters provide a clean opt-in mechanism for early-stop behavior while maintaining backward compatibility.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

ocp_resources/resource.py (1)

1273-1280: Stop-condition matching logic is correct; consider small refactor for Ruff TRY003

The early‑exit logic correctly:

• Builds actual_condition from type and status.
• Checks stop_condition only when explicitly provided.
• Raises ConditionError when {type: stop_condition, status: stop_status} is observed, before evaluating the success condition, which matches the PR objective.

Two minor suggestions:

1. Be explicit that stop_condition ignores reason/message
   Current matching only uses type and status. If future callers ever need to distinguish stop vs success by reason or message, they’ll need a different mechanism. For now this is fine but should be a conscious choice (maybe clarified in higher‑level docs or usage examples).

2. Optionally factor the error message to a helper to satisfy Ruff TRY003
   Ruff’s TRY003 warning is style‑related. If you want to address it and slightly improve reuse/readability, you can move the formatting into a helper on Resource and pass only the computed message in the raise:

@@
     def wait_for_condition(
         self,
@@
-        for sample in TimeoutSampler(
+        for sample in TimeoutSampler(
             wait_timeout=timeout_watcher.remaining_time(),
             sleep=sleep_time,
             func=lambda: self.instance,
         ):
@@
-                    actual_condition = {"type": cond["type"], "status": cond["status"]}
-
-                    if stop_condition:
-                        if actual_condition == {"type": stop_condition, "status": stop_status}:
-                            raise ConditionError(
-                                f"{self.kind} {self.name} reached stop_condition '{stop_condition}' in status '{stop_status}':\n{cond}"
-                            )
+                    actual_condition = {"type": cond["type"], "status": cond["status"]}
+
+                    if stop_condition and actual_condition == {"type": stop_condition, "status": stop_status}:
+                        raise ConditionError(
+                            self._format_stop_condition_error_message(
+                                stop_condition=stop_condition,
+                                stop_status=stop_status,
+                                cond=cond,
+                            )
+                        )
@@
                     if actual_condition == expected_condition and message in cond.get("message", ""):
                         return
+
+    def _format_stop_condition_error_message(
+        self,
+        stop_condition: str,
+        stop_status: str,
+        cond: dict[str, object] | ResourceField,
+    ) -> str:
+        return (
+            f"{self.kind} {self.name} reached stop_condition '{stop_condition}' "
+            f"in status '{stop_status}':\n{cond}"
+        )

This keeps behavior identical while avoiding a long formatted string literal directly in the raise and addressing the Ruff hint.

📜 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 862969a and d2d3102.

📒 Files selected for processing (2)

• ocp_resources/exceptions.py (1 hunks)
• ocp_resources/resource.py (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• ocp_resources/exceptions.py

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2024-10-08T23:43:22.342Z

Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-08T23:43:22.342Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.

Applied to files:

• ocp_resources/resource.py

📚 Learning: 2025-08-11T16:42:27.596Z

Learnt from: servolkov
Repo: RedHatQE/openshift-python-wrapper PR: 2490
File: ocp_resources/route_advertisements.py:5-5
Timestamp: 2025-08-11T16:42:27.596Z
Learning: In the openshift-python-wrapper project, MissingRequiredArgumentError should be imported from ocp_resources.resource (not ocp_resources.exceptions), as it's re-exported there and this is the established pattern used throughout the codebase.

Applied to files:

• ocp_resources/resource.py

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/exceptions.py (1)

• ConditionError (93-96)

🪛 Ruff (0.14.5)

ocp_resources/resource.py

1278-1280: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (2)

ocp_resources/resource.py (2)

42-49: ConditionError import is appropriate and consistent with exception usage

Importing ConditionError here aligns with how other resource-related exceptions are centralized in ocp_resources.exceptions and then consumed from resource.py. No issues with this addition.

---

1235-1262: wait_for_condition API extension is backward compatible and semantically clear

The new stop_condition / stop_status parameters are appended at the end with safe defaults, so existing callers remain unaffected while enabling early‑fail semantics when explicitly requested. The docstring updates clearly document the new behavior and the additional ConditionError failure mode.

[jpeimer]

/lgtm

[kshvaika]

/verified

[rnetser]

/verified Run tests that use wait_for_condition function: for example, storage checkups test: uv run --with-editable ../openshift-python-wrapper pytest -o log_cli=true -s --skip-deprecated-api-test -rsx --tc=no_unprivileged_client:True --cluster-sanity-skip-check tests/storage/checkups/ --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization

should be verified regardless of any repo that is using this repo.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

ocp_resources/resource.py (1)

1277-1281: Logic correctly implements fail-fast behavior.

The stop_condition check is properly placed to detect unexpected states early and fail immediately with a descriptive error message including the full condition dict.

Optional: Consider extracting the error message format.

The static analysis tool suggests extracting long error messages. You could create a helper method or constant for the message format, though the current inline approach is perfectly readable:

def _format_stop_condition_error(self, stop_condition: str, stop_status: str, cond: dict) -> str:
    return (
        f"{self.kind} {self.name} reached stop_condition '{stop_condition}' "
        f"in status '{stop_status}':\n{cond}"
    )

Then use:

-                        raise ConditionError(
-                            f"{self.kind} {self.name} reached stop_condition '{stop_condition}' in status '{stop_status}':\n{cond}"
-                        )
+                        raise ConditionError(
+                            self._format_stop_condition_error(stop_condition, stop_status, cond)
+                        )

Based on learnings: This aligns with Ruff TRY003 guidance, but it's entirely optional—the current implementation is clear and maintainable.

📜 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 d2d3102 and a175dc0.

📒 Files selected for processing (1)

• ocp_resources/resource.py (4 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2024-10-08T23:43:22.342Z

Learnt from: sbahar619
Repo: RedHatQE/openshift-python-wrapper PR: 2139
File: ocp_resources/resource.py:256-257
Timestamp: 2024-10-08T23:43:22.342Z
Learning: Constants added to `Condition.Reason` in `ocp_resources/resource.py` may be intended for future use even if currently unused.

Applied to files:

• ocp_resources/resource.py

📚 Learning: 2025-08-11T16:42:27.596Z

Learnt from: servolkov
Repo: RedHatQE/openshift-python-wrapper PR: 2490
File: ocp_resources/route_advertisements.py:5-5
Timestamp: 2025-08-11T16:42:27.596Z
Learning: In the openshift-python-wrapper project, MissingRequiredArgumentError should be imported from ocp_resources.resource (not ocp_resources.exceptions), as it's re-exported there and this is the established pattern used throughout the codebase.

Applied to files:

• ocp_resources/resource.py

🧬 Code graph analysis (1)

ocp_resources/resource.py (1)

ocp_resources/exceptions.py (1)

• ConditionError (93-96)

🪛 Ruff (0.14.5)

ocp_resources/resource.py

1279-1281: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (3)

ocp_resources/resource.py (3)

44-44: LGTM! Import is necessary for the new functionality.

The ConditionError import is correctly added to support the new early-stop logic in wait_for_condition.

---

1243-1244: Well-designed parameters with sensible defaults.

The new stop_condition and stop_status parameters are backward-compatible and provide the flexibility to fail fast when an unexpected condition is detected.

---

1256-1263: Excellent documentation.

The docstring clearly explains the new parameters and behavior, including the important note that stop_condition matching only uses type and status (ignoring reason and message fields).

[kshvaika]

/verified

[rnetser]

/approve
/lgtm

addressed