Add last automatic backup state sensor #144187
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
|
| # Last backup has failed locations | ||
| return LastBackupState.BACKUP_FAILED_LOCATIONS | ||
|
|
||
| return LastBackupState.BACKUP_SUCCESSFUL |
There was a problem hiding this comment.
I don't like this calculation. It's an arbitrary decision to create just these states from the available data.
I'd rather return the underlying data as is from different sensors or service action calls.
There was a problem hiding this comment.
it's not as arbitrary as it seems, it is inspired from the way how the frontend calculates this state - the difference is, that we use the KnownBackups data to avoid further calls against the backup agents and the KnownBackups data should be reliable, as they are updated every time a backup is createdtriggered
There was a problem hiding this comment.
Still, there's nothing in the underlying data saying we should limit it to these states. If we start calculating states we can come up with lots of possible calculations. I don't think we should go there.
There was a problem hiding this comment.
I agree, that we could add countless states based on calculations, but the intention was to provide the user a sensor which represents equal states, like the frontend does, but with the advantage for the user, that they can create use it as an automation trigger. Unfortunately these states are based on calculations at the moment, as the state of the last automatic backup job is just not fully tracked/stored by the backup manager (we only know, when was the last attempt and when was the last success attempt).
So to not rely on calculations, we could extend the backup manager to track the state of the last automatic backup job - to keep it simple, we could limit this to a binary state true=success, false=failed and none=no state available 🤔
What do you think? Or should we put this into an arch-discussion?
|
I suggest you split the PR in two, one for each new sensor. |
mib1185
changed the title
the last attempted sensor is now split into #144194 - |
| class LastBackupState(StrEnum): | ||
| """Last backup state type.""" | ||
|
|
||
| NO_BACKUP = "no_backup" | ||
| BACKUP_FAILED_LOCATIONS = "last_backup_failed_locations" | ||
| BACKUP_FAILED = "last_backup_failed" | ||
| BACKUP_SUCCESSFUL = "last_backup_successful" |
There was a problem hiding this comment.
I think this, and the calculation in coordinator, should be moved to sensor.py since it's only used by the proposed new sensor
There was a problem hiding this comment.
sounds absolutely reasonable, but I'll wait until the discussion above (#144187 (comment)) is resolved 👍
Proposed change
This adds the
last_automatic_backup_statesensorKnownBackupsdata to avoid further calls to the backup agentsreference: feature request
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: