Add sizes per location and support .local
#5581
Conversation
mdegat01
added
missing-documentation
mdegat01
changed the title
.local
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the backup system in the supervisor API. The changes focus on improving location handling, backup file size calculation, and schema definitions. A new approach to managing backup location attributes is implemented, replacing the previous Changes
Sequence DiagramsequenceDiagram
participant API as Backup API
participant Backup as Backup Manager
participant Storage as Storage System
API->>Backup: Request backup details
Backup->>Storage: Retrieve backup file
Storage-->>Backup: Return file path
Backup->>Backup: Calculate file size
Backup-->>API: Return backup details with location attributes
This sequence diagram illustrates the new flow of retrieving backup details, highlighting the enhanced location attribute generation and file size calculation process. ✨ Finishing Touches
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)
153-161: Consider adding docstring with example return value.The
_make_location_attributesmethod constructs the location attributes dictionary but could benefit from a docstring showing the structure of the returned dictionary.Add a docstring like this:
def _make_location_attributes(self, backup: Backup) -> dict[str, dict[str, Any]]: - """Make location attributes dictionary.""" + """Make location attributes dictionary. + + Example return value: + { + ".local": { + "protected": True, + "size_bytes": 1024 + }, + "backup_mount": { + "protected": False, + "size_bytes": 2048 + } + } + """supervisor/backups/backup.py (1)
231-239: Consider adding docstring with size unit information.The size properties could benefit from docstrings clarifying the units:
sizereturns megabytessize_bytesreturns bytesAdd docstrings like this:
@property def size(self) -> float: - """Return backup size.""" + """Return backup size in megabytes.""" return round(self.size_bytes / 1048576, 2) # calc mbyte @property def size_bytes(self) -> int: - """Return backup size in bytes.""" + """Return backup size in bytes for the current location.""" return self.location_size(self.location)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/api/backups.py(10 hunks)supervisor/api/const.py(1 hunks)supervisor/backups/backup.py(4 hunks)tests/api/test_backups.py(9 hunks)
🔇 Additional comments (9)
supervisor/api/const.py (1)
50-50: LGTM! The new constant follows the naming convention.The addition of
ATTR_LOCATION_ATTRIBUTESand removal ofATTR_PROTECTED_LOCATIONSaligns with the PR objectives to enhance backup location information.supervisor/api/backups.py (4)
70-71: LGTM! The constant defines the key for local backups.The introduction of
LOCATION_LOCALconstant aligns with the PR objectives to use.localas the key for local backups.
87-91: LGTM! The function handles local location values consistently.The
_convert_local_locationfunction correctly implements the requirement to treat both.localand empty string""as references to the local/backupsfolder.
95-97: LGTM! The schema definitions ensure proper validation.The schema constants provide robust validation:
SCHEMA_FOLDERSvalidates folder listsSCHEMA_LOCATIONhandles location conversionSCHEMA_LOCATION_LISTvalidates location lists
103-103: LGTM! The schema updates follow a logical pattern.The schema definitions correctly handle location validation:
- Single location for restore operations using
SCHEMA_LOCATION- Multiple locations for backup and remove operations using
SCHEMA_LOCATION_LISTAlso applies to: 121-121, 140-140
supervisor/backups/backup.py (2)
70-74: LGTM! The function efficiently caches backup file sizes.The
_backup_file_sizefunction is well-designed:
- Uses
lru_cachefor performance optimization- Handles non-existent files gracefully
- Has a clear, single responsibility
263-270: LGTM! The method handles location-specific size retrieval.The
location_sizemethod is well-implemented:
- Returns 0 for non-existent locations
- Uses cached file size calculation
- Has clear error handling
tests/api/test_backups.py (2)
103-109: LGTM! The test parameters cover all location scenarios.The test parameters effectively verify:
- Backup to named location
- Backup to local location using both
Noneand empty string- Backup to local location using
.local
1004-1009: LGTM! The assertions verify location attributes structure.The test assertions thoroughly validate the location attributes:
- Verify correct attribute structure
- Test protection status
- Validate size information
Also applies to: 1038-1043, 1051-1056
| @lru_cache | ||
| def _backup_file_size(backup: Path) -> int: | ||
| """Get backup file size.""" | ||
| return backup.stat().st_size if backup.is_file() else 0 |
There was a problem hiding this comment.
We should get the size when scanning the backup and cache the information. But it used to work like this before, so let's do one thing at a time.
There was a problem hiding this comment.
Nice! This makes .local work for all API, which is very nice. I think we should then also document that this is the preferred identifier.
Maybe we should also consider using .local throughout Supervisor. But this can be done in a follow, if we decide to go down that road.
mdegat01
removed
need-client-library
Proposed change
Add
location_attributesto API. Returns a dictionary of locations that backup can be found in. For each location says if the backup is protected and the size in bytes.This removes the
protected_locationsfield added in #5569 as it provides a more flexible solution. The reason it wasn't done this way to begin with is because the value used for the local/backupsfolder isNoneandNonecannot be a dictionary key in JSON.To handle this we use
.localas the key in this dictionary. For consistency across the APIs I also adjusted all APIs which accept a location to allow.localand""and treat those as the local/backupsfolder.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
protected_locationswith more comprehensivelocation_attributesTests