-
Notifications
You must be signed in to change notification settings - Fork 282
Add ability to configure Postgres backups #3750
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
Conversation
internal/command/postgres/backup.go
Outdated
| return hasRequiredVersionOnMachines(appName, machines, "", "0.0.53", "") | ||
| } | ||
|
|
||
| func hasRequiredVersionForBackupConfig(appName string, machines []*fly.Machine) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Do we not want to block on versions like v0.0.53 with backup support but not backup config support? Happy to remove it but the UX when someone runs fly pg backup config wouldn't clarify how they fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I should have clarified. The version check is necessary, but abstracting it into a separate function isn't, since it's just issuing a return.
internal/command/postgres/backup.go
Outdated
| } | ||
|
|
||
| func hasRequiredVersion(appName string, machines []*fly.Machine) error { | ||
| func hasRequiredVersionForBackup(appName string, machines []*fly.Machine) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do the same thing here that we did below? Define the version as a constant and just reuse hasRequiredVersiononMachines ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment, but once that is fixed feel free to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Change Summary
fly pg backup configsubcommand with ability to show and update backup-related configuration valuesRelated to: fly-apps/postgres-flex#244
Documentation