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

Schedule Heroku Backups and Capture backup of existing staging database for Review Apps #826

Merged
merged 11 commits into from
Mar 24, 2017

Conversation

toobulkeh
Copy link
Contributor

When a PR Review environment is created, the latest backup is currently used. If no backups exist, or if the backup is old and outdated, recent data used for testing may not exist.

This forcibly creates a backup right before migration -- leading to the most recent data.

Another way to solve this would be to automatically add pg:backups:schedule on all staging environments on original app scaffolding, but that would be up to 24 hours difference.

When a PR Review environment is created, the latest backup is currently used. If no backups exist, or if the backup is old and outdated, recent data used for testing may not exist.

This forcibly creates a backup right before migration -- leading to the most recent data.

Another way to solve this would be to automatically add `pg:backups:schedule` on all staging environments on original app scaffolding, but that would be up to 24 hours difference.
@tute
Copy link
Contributor

tute commented Feb 20, 2017

Another way to solve this would be to automatically add pg:backups:schedule on all staging environments on original app scaffolding, but that would be up to 24 hours difference.

This is what I normally do on my Rails projects. Regardless of how we merge this PR, I'd like production and staging to have daily backups by default (I thought this was already the case).

I don't feel strongly either way, and we can merge as is or instead turn this PR into adding the daily backup instruction on app creation, and then keeping this script as is. What do you prefer?

Thanks for sharing your work!

@toobulkeh
Copy link
Contributor Author

both

)
end
end

Choose a reason for hiding this comment

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

Trailing whitespace detected.

)
end
end

Choose a reason for hiding this comment

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

Trailing whitespace detected.


def have_backup_schedule(remote_name)
have_received(:run).
with(/pg:backups:schedule DATABASE_URL --at '02:00 America\/Los_Angeles' --remote #{remote_name}/)

Choose a reason for hiding this comment

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

Line is too long. [108/80]

@@ -48,6 +58,11 @@ module Adapters
def app_name
SuspendersTestHelpers::APP_NAME
end

Choose a reason for hiding this comment

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

Trailing whitespace detected.

expect(app_builder).to have_backup_schedule("staging")
expect(app_builder).to have_backup_schedule("production")
end

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@toobulkeh toobulkeh changed the title Capture backup of existing staging database Schedule Heroku Backups and Capture backup of existing staging database for Review Apps Feb 21, 2017
@tute
Copy link
Contributor

tute commented Feb 21, 2017

This LGTM. Calling in @iwz for another review before I merge. Thank you! :)

Copy link
Contributor

@iwz iwz 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 overall, but that LA time zone feels a bit out of place

def set_heroku_backup_schedule
%w(staging production).each do |environment|
run_toolbelt_command(
"pg:backups:schedule DATABASE_URL --at '02:00 America/Los_Angeles'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps convert this to UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the Heroku default recommended in their docs -- to convert it do you mean 2AM UTC or 10AM UTC?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I saw that in their docs. 10AM UTC would be fine.

@@ -49,6 +59,11 @@ def app_name
SuspendersTestHelpers::APP_NAME
end

def have_backup_schedule(remote_name)
have_received(:run).
with(/pg:backups:schedule DATABASE_URL --at '02:00 America\/Los_Angeles' --remote #{remote_name}/)

Choose a reason for hiding this comment

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

Line is too long. [108/80]

@@ -49,6 +59,11 @@ def app_name
SuspendersTestHelpers::APP_NAME
end

def have_backup_schedule(remote_name)
have_received(:run).
with(/pg:backups:schedule DATABASE_URL --at '10:00 UTC' --remote #{remote_name}/)

Choose a reason for hiding this comment

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

Line is too long. [91/80]

@iwz
Copy link
Contributor

iwz commented Feb 22, 2017

LGTM. I'm not sure what others think about the hound line wrapping, though...

@tute @croaky @jferris

@iwz
Copy link
Contributor

iwz commented Mar 24, 2017

@toobulkeh this looks good to go. Do you want to clean up any commits before we merge? Also, do you think the README should mention anything about the scheduled backups?

@toobulkeh
Copy link
Contributor Author

Added to the README @iwz. I'd appreciate a Squash and Merge. Is that something I can do?

@iwz iwz merged commit f00f151 into thoughtbot:master Mar 24, 2017
@iwz
Copy link
Contributor

iwz commented Mar 24, 2017

@toobulkeh done. thanks for implementing this!

@croaky
Copy link
Contributor

croaky commented Mar 24, 2017

Ooh ooh ooh, good one.

Shall we bump the gem version?

@iwz
Copy link
Contributor

iwz commented Mar 27, 2017

@croaky good idea. i'm not set up for that atm, are you?

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.

5 participants