Skip to content

feat: Stream backups#6587

Open
regdocs wants to merge 1 commit into
developfrom
stream-backups
Open

feat: Stream backups#6587
regdocs wants to merge 1 commit into
developfrom
stream-backups

Conversation

@regdocs
Copy link
Copy Markdown
Member

@regdocs regdocs commented Jun 2, 2026

  • Ensure backup bucket's REGION in agent payload

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR fixes the get_backup_bucket helper to return a uniform dict for both the cluster-specific and default-bucket code paths, and adds REGION and PROVIDER keys to the offsite backup auth payload sent to the agent.

  • site_backup.py: replaces the old mixed str | dict return with a consistent dict and correctly reads backup_region (instead of the non-existent bucket_region) from Press Settings; agent.py is updated to drop the old isinstance branching and call .get() directly.
  • agent.py: PROVIDER is now included in the auth dict, but it will be None for any site whose cluster has a dedicated Backup Bucket record, because that doctype has no provider field and the frappe.get_all query does not fetch one.

Confidence Score: 4/5

Safe to merge for the default-bucket path; the cluster-specific bucket path will always send PROVIDER=None to the agent.

The two main regressions from the old code (wrong field name bucket_region and the dict/string type mismatch) are correctly fixed. However, the newly introduced PROVIDER key in the auth payload will always be None for cluster-specific buckets because Backup Bucket has no provider field.

press/press/doctype/site_backup/site_backup.py — the get_backup_bucket return value for the cluster-specific branch omits provider.

Important Files Changed

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
Loading

Reviews (2): Last reviewed commit: "feat: Stream backups" | Re-trigger Greptile

Comment thread press/press/doctype/site_backup/site_backup.py
Comment thread press/press/doctype/site_backup/site_backup.py Outdated
Comment thread press/press/doctype/site_backup/site_backup.py
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.91%. Comparing base (4815fa4) to head (fe5c05f).
⚠️ Report is 319 commits behind head on develop.

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     
Flag Coverage Δ
dashboard 60.07% <ø> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 563 to +574
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"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 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.

Suggested change
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"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant