Fix Supervisor log fallback for the /follow endpoint #5354
Conversation
When an error occurs when streaming Supervisor logs, the fallback method receives the follow kwarg as well, which is invalid for the Docker log handler: TypeError: APISupervisor.logs() got an unexpected keyword argument 'follow' The exception is still printed to the logs but with all the extra noise caused by this error. Removing the argument makes the stack trace more comprehensible and the fallback actually works as desired.
📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant SupervisorLogs
Client->>API: Request supervisor logs
API->>SupervisorLogs: Call get_supervisor_logs()
SupervisorLogs-->>API: Return logs
API-->>Client: Send logs response
sequenceDiagram
participant Client
participant API
participant SupervisorLogs
Client->>API: Request follow logs
API->>SupervisorLogs: Call get_supervisor_logs()
alt Success
SupervisorLogs-->>API: Return logs
API-->>Client: Send logs response
else Error
API-->>Client: Send error response
end
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/api/test_supervisor.py (2)
186-196: LGTM! Consider adding content verification.The new test case for the
/supervisor/logs/followendpoint is well-structured and improves the overall test coverage. It correctly checks for the expected status code and content type.Consider adding a check for the content of the response, similar to the test for the non-follow endpoint. This would ensure that the fallback mechanism is returning the expected log data. For example:
content = await resp.read() assert content.startswith(b"\x1b[36m") # Check for the expected color code
Line range hint
214-233: LGTM! Consider adding a positive test case.The modifications to
test_api_supervisor_fallback_log_captureimprove the test coverage by differentiating between expected and unexpected errors. The test now correctly verifies that Sentry log capture is only called for unexpected errors.Consider adding a positive test case where no exception is raised to ensure that the normal flow doesn't trigger Sentry log capture. For example:
journald_logs.side_effect = None journald_logs.return_value = "Some log data" with patch("supervisor.api.capture_exception") as capture_exception: await api_client.get("/supervisor/logs") capture_exception.assert_not_called()This would provide complete coverage of all possible scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- supervisor/api/init.py (1 hunks)
- tests/api/test_supervisor.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
tests/api/test_supervisor.py (2)
Line range hint
198-211: LGTM! Improved error handling coverage.The modifications to include a check for
OSErrorin addition toHassioErrorenhance the robustness of the test case. This change ensures that the fallback mechanism works correctly for different types of exceptions that might occur during log retrieval.
Line range hint
1-233: Overall assessment: Excellent improvements to test coverage and error handlingThe changes in this file significantly enhance the test suite for the Supervisor API, particularly in relation to log retrieval and error handling. The new test case for the
/supervisor/logs/followendpoint and the modifications to existing tests align well with the PR objectives of fixing the Supervisor log fallback mechanism.Key improvements:
- Added coverage for the
/supervisor/logs/followendpoint.- Enhanced error handling tests to cover both
HassioErrorandOSError.- Improved Sentry log capture tests to differentiate between expected and unexpected errors.
These changes contribute to a more robust and comprehensive test suite, which will help maintain the reliability of the Supervisor API.
supervisor/api/__init__.py (1)
416-417: Properly handling the 'follow' parameter in the fallback.The removal of the
'follow'parameter fromkwargsbefore callingapi_supervisor.logsensures that no unexpected keyword arguments are passed to the fallback method, preventing aTypeError. This is a good fix for the issue described.
Proposed change
When an error occurs when streaming Supervisor logs, the fallback method receives the follow kwarg as well, which is invalid for the Docker log handler:
TypeError: APISupervisor.logs() got an unexpected keyword argument 'follow'
The exception is still printed to the logs but with all the extra noise caused by this error. Removing the argument makes the stack trace more comprehensible and the fallback actually works as desired.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation