Add all addons flag to partial backups #5490
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new feature for partial backups in the Supervisor's backup system. A new constant Changes
Sequence DiagramsequenceDiagram
participant Client
participant BackupAPI
participant AddonManager
participant BackupSystem
Client->>BackupAPI: Request partial backup with ALL flag
BackupAPI->>AddonManager: Get list of local addons
AddonManager-->>BackupAPI: Return all addon names
BackupAPI->>BackupSystem: Initiate backup with addon list
BackupSystem-->>BackupAPI: Create backup
BackupAPI-->>Client: Return backup details
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
🧹 Nitpick comments (2)
supervisor/api/backups.py (1)
359-361: Consider validating the list of local addons.While the conversion from
ALL_ADDONS_FLAGto a list of local addons is clean, consider adding validation to ensure the list is not empty and that all addons are in a valid state for backup.if body.get(ATTR_ADDONS) == ALL_ADDONS_FLAG: - body[ATTR_ADDONS] = list(self.sys_addons.local) + local_addons = list(self.sys_addons.local) + if not local_addons: + raise APIError("No local addons found to backup") + body[ATTR_ADDONS] = local_addonstests/api/test_backups.py (1)
797-812: Consider adding more test cases.The test covers the happy path well, but consider adding tests for:
- Empty addon list scenario
- Error handling when addons are in invalid states
- Validation of the response structure
@pytest.mark.parametrize( "addons_list,expected_status", [ ([], 400), # Empty addon list ([Addon(invalid_state=True)], 400), # Invalid addon state ] ) async def test_partial_backup_all_addons_error_cases( api_client: TestClient, coresys: CoreSys, addons_list: list[Addon], expected_status: int, ): """Test error cases for ALL addons backup.""" coresys.core.state = CoreState.RUNNING coresys.hardware.disk.get_disk_free_space = lambda x: 5000 with patch.object(CoreSys, "addons") as mock_addons: mock_addons.local = addons_list resp = await api_client.post( "/backups/new/partial", json={"name": "All addons test", "addons": "ALL"} ) assert resp.status == expected_status
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supervisor/api/backups.py(3 hunks)tests/api/test_backups.py(15 hunks)
🔇 Additional comments (2)
supervisor/api/backups.py (2)
64-65: LGTM! Well-defined constant.
The constant follows Python naming conventions and is self-descriptive.
113-115: LGTM! Schema validation looks good.
The schema modification correctly handles both the new flag and the original list format while maintaining validation.
Proposed change
Allow specifying
ALLfor addons when making a partial backup via the API. Supervisor will then collect the current list of addons and include them all. This is to facilitate scheduled backups where the user wants all addons included in every backup even though the list of addons may change over time.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
Tests