Handle backup delete after config #131259

MartinHjelmare · 2024-11-22T12:38:55Z

Breaking change

Proposed change

Allow the user to set a delete after config for automatic backups. The delete after config will clean up old backups according to the settings.
The possible settings are:
- copies
- days
The copies setting will delete backups after the number of set copies. This clean up will be done after each new backup is created.
The days setting will delete backups older than the number of set days. This clean up will be done once a day and if the setting changes.
The last backup can never be cleaned up automatically even if the settings try to do that.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-11-22T12:39:01Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (backup) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of backup can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign backup Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

homeassistant/components/backup/__init__.py

homeassistant/components/backup/config.py

emontnemery

Looks good! Some minor comments. I've not reviewed the tests yet.

homeassistant/components/backup/config.py

emontnemery · 2024-11-22T13:02:05Z

homeassistant/components/backup/config.py

+                return {
+                    backup_id: backup
+                    for backup_id, backup in backups.items()
+                    if dt_util.parse_datetime(backup.date, raise_on_error=True)


As a side note: It's a bit silly to have to parse the time stamps, I think we should update the dataclass to store a datetime object?

It was convenient with strings when writing the tests now. 😃

I haven't checked if we use the date for anything else than this clean up at the moment.

homeassistant/components/backup/config.py

emontnemery · 2024-11-22T13:31:32Z

One more comment: I think naming this feature and the related classes "backup retention" would be clearer than "backup delete after". We can always adjust that in a follow-up though if we want to get this merged.

Co-authored-by: Erik Montnemery <erik@montnemery.com>

MartinHjelmare · 2024-11-22T17:20:38Z

One more comment: I think naming this feature and the related classes "backup retention" would be clearer then "backup delete after". We can always adjust that in a follow-up though if we want to get this merged.

I'm not sure. It's quite clear when looking at the websocket command payload and it corresponds well to what we're doing: Deleting backups after either a count of days or a count of backups.

emontnemery

Thanks @MartinHjelmare 👍

MartinHjelmare requested a review from a team as a code owner November 22, 2024 12:38

home-assistant bot added cla-signed has-tests integration: backup new-feature by-code-owner Quality Scale: internal labels Nov 22, 2024

MartinHjelmare commented Nov 22, 2024

View reviewed changes

homeassistant/components/backup/__init__.py Show resolved Hide resolved

MartinHjelmare commented Nov 22, 2024

View reviewed changes

homeassistant/components/backup/config.py Outdated Show resolved Hide resolved

MartinHjelmare commented Nov 22, 2024

View reviewed changes

homeassistant/components/backup/config.py Outdated Show resolved Hide resolved

emontnemery reviewed Nov 22, 2024

View reviewed changes

MartinHjelmare and others added 15 commits November 22, 2024 18:00

Handle delete after copies

54277a4

Handle delete after days

13907d3

Add some test examples

48e9ec4

Test config_delete_after_logic

b9402cf

Test config_delete_after_copies_logic

c37b926

Test more delete after days

c2cb9db

Add debug logs

86affec

Always delete the oldest backup first

f1cd60e

Never remove the last backup

9b122c3

Clean up words

3ce8a57

Co-authored-by: Erik Montnemery <erik@montnemery.com>

Fix after cleaning words

2999998

Use utcnow

cd35d11

Remove duplicate guard

a7b4c72

Simplify sorting

586f0d9

Delete backups even if there are agent errors on get backups

7d358d7

MartinHjelmare force-pushed the cloud-backup-handle-delete-after branch from e13a4c9 to 7d358d7 Compare November 22, 2024 17:22

emontnemery approved these changes Nov 23, 2024

View reviewed changes

emontnemery merged commit ce2f990 into allthebackupchanges Nov 23, 2024
36 checks passed

emontnemery deleted the cloud-backup-handle-delete-after branch November 23, 2024 14:15

emontnemery mentioned this pull request Nov 23, 2024

Rename backup delete after to backup retention #131364

Merged

19 tasks

github-actions bot locked and limited conversation to collaborators Nov 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle backup delete after config #131259

Handle backup delete after config #131259

MartinHjelmare commented Nov 22, 2024 •

edited

Loading

home-assistant bot commented Nov 22, 2024

emontnemery left a comment

emontnemery Nov 22, 2024 •

edited

Loading

MartinHjelmare Nov 22, 2024

emontnemery commented Nov 22, 2024 •

edited

Loading

MartinHjelmare commented Nov 22, 2024

emontnemery left a comment

Handle backup delete after config #131259

Handle backup delete after config #131259

Conversation

MartinHjelmare commented Nov 22, 2024 • edited Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Nov 22, 2024

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

MartinHjelmare Nov 22, 2024

Choose a reason for hiding this comment

emontnemery commented Nov 22, 2024 • edited Loading

MartinHjelmare commented Nov 22, 2024

emontnemery left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Nov 22, 2024 •

edited

Loading

emontnemery Nov 22, 2024 •

edited

Loading

emontnemery commented Nov 22, 2024 •

edited

Loading