Make local backup a backup agent #130623
Make local backup a backup agent #130623
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| class LocalBackupAgent(BackupAgent): | ||
| """Define the format that backup agents can have.""" | ||
|
|
||
| name = "local" |
There was a problem hiding this comment.
In a separate PR, we may want to make name human readable and add a slug-type id.
| if data_file := backup_file.extractfile("./backup.json"): | ||
| data = json_loads_object(data_file.read()) | ||
| backup = LocalBackup( | ||
| id=cast(str, data["slug"]), # Do we need another ID? |
There was a problem hiding this comment.
The UploadedBackup base class says we need an ID, I'm not sure why, manager and frontend should care about the slug only.
There was a problem hiding this comment.
The id is the agent's way of identifying the backup. The slug is something that the manager uses to identify the backup. They could be the same but I think it's good to separate terms since the semantics are different.
There was a problem hiding this comment.
OK, but if we don't need the additional identifier there's no point in storing a reference to the slug twice.
The manager will only ever ask the agent to handle a backup based on the slug. The ID is an implementation detail which individual agents can add if they need one.
There was a problem hiding this comment.
Right now the id is part of the interface and the id parameter is used to eg download a backup. We could remove it and force agents to keep track of backups via the slug.
I don't like the name slug since it doesn't describe the purpose of the parameter so I'd prefer to limit its use in interfaces.
There was a problem hiding this comment.
OK.
I suggest we rename it to backup_id which makes the purpose clear.
Local agents can then add their own ID if needed for internal purpose.
| async def async_get_backups(self, **kwargs: Any) -> dict[str, Backup]: | ||
| """Return backups.""" |
There was a problem hiding this comment.
I think it makes sense the manager knows about all backups across all agents.
The manager should not have to ask each time though, there needs to be some caching. Also, either frontend or an agent should be able to initiate a refresh.
There was a problem hiding this comment.
The caching needs to happen in the individual agents, they know what's reasonable, not the manager
|
|
||
| return backup | ||
|
|
||
| async def async_remove_backup(self, *, slug: str, **kwargs: Any) -> None: |
There was a problem hiding this comment.
This isn't part of the agent interface yet. I'm working on adding async_delete_backup.
There was a problem hiding this comment.
We can keep it for now. I'll replace it later.
|
|
||
| if backup is None or not backup.path.exists(): | ||
| if backup is None or path is None or not path.exists(): |
There was a problem hiding this comment.
We need to run the exists check in the executor.
There was a problem hiding this comment.
I'd replace this check with try... except FileNotFoundError when reading the file though since the file can be removed at any time.
There was a problem hiding this comment.
It seems aiohttp already checks that, we don't need to do it
| @@ -42,12 +42,13 @@ async def get( | |||
|
|
|||
| manager = cast(BackupManager, request.app[KEY_HASS].data[DATA_MANAGER]) | |||
| backup = await manager.async_get_backup(slug=slug) | |||
| path = await manager.async_get_backup_path(slug=slug) | |||
There was a problem hiding this comment.
I'd expect us to download the backup from the agent here. If the agent already has it downloaded it doesn't need to do it again of course. The downloaded backup will always have a local path on disk.
| @@ -48,8 +49,10 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: | |||
|
|
|||
| async def async_handle_create_service(call: ServiceCall) -> None: | |||
| """Service handler for creating backups.""" | |||
| agent_id = list(backup_manager.local_backup_agents)[0] | |||
There was a problem hiding this comment.
Side note: We should probably handle missing local agents here and raise HomeAssistantError.
| manager = hass.data[DATA_MANAGER] | ||
| await manager.load_platforms() | ||
|
|
||
| manager = cast(BackupManager, hass.data[DATA_MANAGER]) |
There was a problem hiding this comment.
Will this change later or can we allow the base manager instead?
There was a problem hiding this comment.
We should review the BackupManager base class as part of adding a hassio backup manager, then this cast should be removed.
| return result | ||
|
|
||
| local_agent = hass.data[DATA_MANAGER].backup_agents[LOCAL_AGENT_ID] | ||
| local_agent._backups = {backups.slug: backups for backups in backups} |
There was a problem hiding this comment.
| local_agent._backups = {backups.slug: backups for backups in backups} | |
| local_agent._backups = {backup.slug: backup for backup in backups} |
|
The test failure is unrelated to this PR |
Proposed change
Make local backup a backup agent
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: