feat: Stream backups#6587
Conversation
regdocs
commented
Jun 2, 2026
- Ensure backup bucket's REGION in agent payload
|
| Filename | Overview |
|---|---|
| press/press/doctype/site_backup/site_backup.py | get_backup_bucket now returns a consistent dict for both paths and correctly uses backup_region, but the cluster-specific branch never populates provider since Backup Bucket has no such field and it isn't fetched. |
| press/agent.py | _get_offsite_backup_config simplified to always call .get() on the dict; adds PROVIDER to the auth payload, but that key will be None for any cluster-specific bucket. |
Sequence Diagram
sequenceDiagram
participant Agent as agent.py _get_offsite_backup_config
participant GBB as site_backup.py get_backup_bucket
participant DB as Frappe DB
Agent->>GBB: "get_backup_bucket(cluster, region=True)"
GBB->>DB: get_all(Backup Bucket, cluster, name+region)
DB-->>GBB: bucket_for_cluster (may be empty)
GBB->>DB: get_single_value Press Settings backup_region
GBB->>DB: get_single_value Press Settings aws_s3_bucket
GBB->>DB: get_single_value Press Settings offsite_backups_provider
DB-->>GBB: default_bucket_config
alt cluster bucket exists
GBB-->>Agent: bucket_for_cluster[0] name+region only, provider missing
else no cluster bucket
GBB-->>Agent: default_bucket_config name+region+provider
end
Agent->>Agent: build auth ACCESS_KEY SECRET_KEY REGION PROVIDER
Agent-->>Agent: return bucket auth path
Reviews (2): Last reviewed commit: "feat: Stream backups" | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6587 +/- ##
===========================================
+ Coverage 49.76% 49.91% +0.15%
===========================================
Files 955 956 +1
Lines 78917 79324 +407
Branches 361 376 +15
===========================================
+ Hits 39272 39598 +326
- Misses 39621 39698 +77
- Partials 24 28 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| bucket_for_cluster = frappe.get_all("Backup Bucket", {"cluster": cluster}, ["name", "region"], limit=1) | ||
| default_bucket = frappe.db.get_single_value("Press Settings", "aws_s3_bucket") | ||
|
|
||
| default_bucket_config = { | ||
| "region": frappe.db.get_single_value("Press Settings", "backup_region"), | ||
| "name": frappe.db.get_single_value("Press Settings", "aws_s3_bucket"), | ||
| "provider": frappe.db.get_single_value("Press Settings", "offsite_backups_provider"), | ||
| } | ||
|
|
||
| if region: | ||
| return bucket_for_cluster[0] if bucket_for_cluster else default_bucket | ||
| return bucket_for_cluster[0]["name"] if bucket_for_cluster else default_bucket | ||
| return bucket_for_cluster[0] if bucket_for_cluster else default_bucket_config | ||
|
|
||
| return bucket_for_cluster[0]["name"] if bucket_for_cluster else default_bucket_config["name"] |
There was a problem hiding this comment.
When a cluster-specific
Backup Bucket is found, the frappe.get_all query only fetches ["name", "region"], and the Backup Bucket doctype has no provider field at all (confirmed in backup_bucket.json). So backup_bucket_config.get("provider") will always be None for that branch, even though the PR intends to send the correct PROVIDER to the agent. Adding provider to the field list won't help here since the doctype doesn't define it — the fallback should be the Press Settings value. A minimal fix is to fetch the provider once and include it in both paths.
| bucket_for_cluster = frappe.get_all("Backup Bucket", {"cluster": cluster}, ["name", "region"], limit=1) | |
| default_bucket = frappe.db.get_single_value("Press Settings", "aws_s3_bucket") | |
| default_bucket_config = { | |
| "region": frappe.db.get_single_value("Press Settings", "backup_region"), | |
| "name": frappe.db.get_single_value("Press Settings", "aws_s3_bucket"), | |
| "provider": frappe.db.get_single_value("Press Settings", "offsite_backups_provider"), | |
| } | |
| if region: | |
| return bucket_for_cluster[0] if bucket_for_cluster else default_bucket | |
| return bucket_for_cluster[0]["name"] if bucket_for_cluster else default_bucket | |
| return bucket_for_cluster[0] if bucket_for_cluster else default_bucket_config | |
| return bucket_for_cluster[0]["name"] if bucket_for_cluster else default_bucket_config["name"] | |
| bucket_for_cluster = frappe.get_all("Backup Bucket", {"cluster": cluster}, ["name", "region"], limit=1) | |
| press_settings = frappe.db.get_value( | |
| "Press Settings", | |
| None, | |
| ["backup_region", "aws_s3_bucket", "offsite_backups_provider"], | |
| as_dict=True, | |
| ) | |
| default_bucket_config = { | |
| "region": press_settings.backup_region, | |
| "name": press_settings.aws_s3_bucket, | |
| "provider": press_settings.offsite_backups_provider, | |
| } | |
| if region: | |
| if bucket_for_cluster: | |
| # Backup Bucket has no provider field; fall back to Press Settings provider | |
| return {**bucket_for_cluster[0], "provider": press_settings.offsite_backups_provider} | |
| return default_bucket_config | |
| return bucket_for_cluster[0]["name"] if bucket_for_cluster else default_bucket_config["name"] |