Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if backup is still available when accessing #5521

Closed
wants to merge 1 commit into from

Conversation

agners
Copy link
Member

@agners agners commented Jan 2, 2025

Proposed change

When accessing a backup (e.g. to restore, download or delete it) check if the locations of the backup file are still available. If not, force a backup reload instead.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • 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:
  • Link to cli pull request:
  • Link to client library 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
  • The code has been formatted using Ruff (ruff format supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints or add-on configuration are added/changed:

@agners agners added the new-feature A new feature label Jan 2, 2025
@agners agners requested a review from mdegat01 January 2, 2025 21:23
Copy link
Collaborator

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except for the method name. That could be changed in a follow-up though.

@@ -143,11 +143,26 @@ def _ensure_list(item: Any) -> list:
class APIBackups(CoreSysAttributes):
"""Handle RESTful API for backups functions."""

def _extract_slug(self, request):
async def _extract_slug(self, request):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method should be renamed since it does (and already did before this PR) more than just get the slug from the request

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to _get_backup_by_slug().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I would actually prefer we don't rename it. Or at the very least, please keep _extract at the beginning. We can change it to _extract_backup_by_slug if we prefer. But if you look at all the other files under api, every one that takes an id of something in the resource path has a matching _extract_<x> method that extracts that id and returns the resource or raises. I'd like to keep that consistency to make these easy to find.

When accessing a backup (e.g. to restore, download or delete it) check
if the locations of the backup file are still available. If not, force
a backup reload instead.
@agners agners force-pushed the reload-backup-if-file-location-not-available branch from 97353d7 to b68d381 Compare January 14, 2025 13:52
@agners
Copy link
Member Author

agners commented Jan 22, 2025

This global refresh is not ideal in any case. We've decided to handle missing backups on Core side, and on Supervisor side just update the cache for the backup in question specifically.

@agners agners closed this Jan 22, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants