Allow core to mark addons as system managed #5145
Conversation
|
Warning Rate limit exceeded@mdegat01 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 33 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThis update enhances Home Assistant’s addon management system by introducing new attributes and properties specifically for system-managed addons. The changes span multiple files, adding these attributes to relevant classes, functions, and validation scripts, while also introducing tests to ensure the correct functionality and state management of addons. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
mdegat01
force-pushed
the
system-managed-addons
branch
from
4b3e9a9 to
ca213cc
Compare
|
Normally when the API is updated with something new in a non-GET method we also update the CLI. However in this case I think we should skip that. I see no valid use case to allowing CLI users to mark an addon as system managed, this option is only for Home Assistant to use via the API. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
supervisor/addons/addon.py (1)
368-398: Consider documenting the new properties in the class docstring.Adding
system_managedandsystem_managed_config_entryto the class docstring would improve readability and maintainability.
tests/api/test_addons.py
Outdated
| async def test_api_addon_system_managed( | ||
| api_client: TestClient, | ||
| coresys: CoreSys, | ||
| install_addon_example: Addon, | ||
| caplog: pytest.LogCaptureFixture, | ||
| tmp_supervisor_data, | ||
| path_extern, | ||
| ): | ||
| """Test setting system managed for an addon.""" | ||
| install_addon_example.data["ingress"] = False | ||
|
|
||
| # Not system managed | ||
| resp = await api_client.get("/addons") | ||
| body = await resp.json() | ||
| assert body["data"]["addons"][0]["slug"] == "local_example" | ||
| assert body["data"]["addons"][0]["system_managed"] is False | ||
|
|
||
| resp = await api_client.get("/addons/local_example/info") | ||
| body = await resp.json() | ||
| assert body["data"]["system_managed"] is False | ||
| assert body["data"]["system_managed_config_entry"] is None | ||
|
|
||
| # Mark as system managed | ||
| coresys.addons.data.save_data.reset_mock() | ||
| resp = await api_client.post( | ||
| "/addons/local_example/options", | ||
| json={"system_managed": True, "system_managed_config_entry": "abc123"}, | ||
| ) | ||
| assert resp.status == 200 | ||
| coresys.addons.data.save_data.assert_called_once() | ||
|
|
||
| resp = await api_client.get("/addons") | ||
| body = await resp.json() | ||
| assert body["data"]["addons"][0]["system_managed"] is True | ||
|
|
||
| resp = await api_client.get("/addons/local_example/info") | ||
| body = await resp.json() | ||
| assert body["data"]["system_managed"] is True | ||
| assert body["data"]["system_managed_config_entry"] == "abc123" | ||
|
|
||
| # Revert. Log that cannot have a config entry if not system managed | ||
| coresys.addons.data.save_data.reset_mock() | ||
| resp = await api_client.post( | ||
| "/addons/local_example/options", | ||
| json={"system_managed": False, "system_managed_config_entry": "abc123"}, | ||
| ) | ||
| assert resp.status == 200 | ||
| coresys.addons.data.save_data.assert_called_once() | ||
| assert "Ignoring system managed config entry" in caplog.text | ||
|
|
||
| resp = await api_client.get("/addons") | ||
| body = await resp.json() | ||
| assert body["data"]["addons"][0]["system_managed"] is False | ||
|
|
||
| resp = await api_client.get("/addons/local_example/info") | ||
| body = await resp.json() | ||
| assert body["data"]["system_managed"] is False | ||
| assert body["data"]["system_managed_config_entry"] is None |
There was a problem hiding this comment.
Comprehensive Testing of System Managed Addons
The test function test_api_addon_system_managed is well-structured and covers multiple scenarios, including setting an addon as system-managed and reverting it. The usage of reset_mock and assert_called_once ensures that changes are being tracked correctly. However, consider adding more edge cases, such as attempting to set invalid values for system_managed and system_managed_config_entry.
Would you like me to help by adding these additional test cases?
mdegat01
force-pushed
the
system-managed-addons
branch
from
fc65c8b to
86dc0cf
Compare
Proposed change
Allow Home Assistant to mark addons as system managed when an integration is handling installation and updates for it.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints of add-on configuration are added/changed:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests