raise if __exit__ fail #2294
raise if __exit__ fail #2294
Conversation
WalkthroughThis pull request introduces a new exception class Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
571-572: Add docstring to explain the new error handling behavior.The new error handling in
__exit__should be documented to explain when and whyResourceTeardownErroris raised.Add a docstring to explain the behavior:
def __exit__( self, exc_type: type[BaseException] | None = None, exc_val: BaseException | None = None, exc_tb: TracebackType | None = None, ) -> None: + """Exit the runtime context and clean up the resource. + + Args: + exc_type: The type of the exception that was raised. + exc_val: The instance of the exception that was raised. + exc_tb: The traceback of the exception that was raised. + + Raises: + ResourceTeardownError: If resource cleanup fails. + """ if self.teardown: if not self.clean_up(): + # Raise an error if cleanup fails to ensure resource teardown issues are not silently ignored raise ResourceTeardownError(resource=self)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ocp_resources/exceptions.py(2 hunks)ocp_resources/resource.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ocp_resources/exceptions.py (1)
58-63:⚠️ Potential issueFix method name and add missing type hints and docstring.
The
ResourceTeardownErrorclass has several issues:
- Method name is incorrect:
_str__instead of__str__.- Typo in error message: "excute" instead of "execute".
- Missing type hint for
__str__return type.- Missing docstring.
Apply this diff to fix the issues:
class ResourceTeardownError(Exception): + """Exception raised when resource teardown fails.""" + def __init__(self, resource: Any): self.resource = resource - def _str__(self): - return f"Failed to excute teardown for resource {self.resource}" + def __str__(self) -> str: + return f"Failed to execute teardown for resource {self.resource}"
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
746-752: Fix typo in warning message.There's a typo in the warning message: "Timeour" should be "Timeout".
Apply this diff to fix the typo:
- self.logger.warning(f"Timeour expired while waiting for {self.kind} {self.name} to be deleted") + self.logger.warning(f"Timeout expired while waiting for {self.kind} {self.name} to be deleted")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ocp_resources/exceptions.py(1 hunks)ocp_resources/resource.py(3 hunks)tests/test_resources.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (4)
tests/test_resources.py (3)
12-17: Well-structured test helper class!The
TestSecretExitclass is well designed for testing error cases, withclean_upalways returningFalseto simulate cleanup failures.
96-100: Good test coverage for the happy path!The test verifies that resources are properly cleaned up when the context manager exits successfully.
102-105: Good test coverage for the error path!The test properly verifies that
ResourceTeardownErroris raised when resource cleanup fails.ocp_resources/resource.py (1)
571-572: Good enhancement to error handling!The change improves error visibility by raising
ResourceTeardownErrorwhen resource cleanup fails.
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (2)
ocp_resources/resource.py (2)
9-14: LGTM! Well-organized import statements.The new imports are logically organized and support the enhanced error handling functionality, particularly the addition of
ResourceTeardownErrorfor improved resource cleanup error reporting.Also applies to: 19-34, 37-38, 44-46
571-572: LGTM! Appropriate error handling in exit.Raising the error in
__exit__rather thanclean_upis the right choice because:
- It maintains separation of concerns -
clean_upfocuses on cleanup logic while__exit__handles error reporting- It follows the context manager protocol where
__exit__is responsible for handling exceptions- It allows
clean_upto be used independently without forcing error handling behavior
When we call
__exit__we do not raise ifdeletefail which is bad and can hide error.Added raise
ResourceTeardownErrorifdeletedo not reurnTruein__exit__.Summary by CodeRabbit
New Features
ResourceTeardownError.Bug Fixes
Refactor
Tests