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.

Sign up for GitHub

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

Jump to bottom

Allow bypass of confirmation when restoring to staging #41

Closed
wants to merge 1 commit into from
Closed

Allow bypass of confirmation when restoring to staging #41

wants to merge 1 commit into from

Conversation

geoffharcourt
Copy link
Collaborator

Heroku asks for confirmation with --confirm appname when performing
potentially destructive CLI operations such as a database restore. If
staging data is tracking with production or easily rebuilt using seeds,
this confirmation can be annoying, particularly during the common
operation of taking a snapshot of production and then immediately
copying it to staging to sync staging's DB up with production's.

This change changes the behavior of non-production, non-local
development environments to append the confirmation argument on database
restores to overwrite the staging or other feature environment database
when restored from another environment.

Close #38.

houndci-bot
houndci-bot reviewed Jul 10, 2015
View reviewed changes
spec/environment_spec.rb
@@ -31,6 +37,24 @@
expect(backup).to have_received(:restore)
end

it "passes the confirmation argument if the application's Parity configuration allows it" do
Copy link

Choose a reason for hiding this comment

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

Line is too long. [94/80]

houndci-bot
houndci-bot reviewed Jul 10, 2015
View reviewed changes
spec/environment_spec.rb
@@ -31,6 +37,24 @@
expect(backup).to have_received(:restore)
end

it "passes the confirmation argument if the app's Parity configuration allows it" do
Copy link

Choose a reason for hiding this comment

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

Line is too long. [86/80]

@geoffharcourt geoffharcourt force-pushed the bypass-confirmation branch 2 times, most recently from 7bd4fde to 4c090bd Compare July 10, 2015 15:55
gabebw
gabebw reviewed Jul 10, 2015
View reviewed changes
README.md
@@ -106,6 +106,8 @@ Override some of the conventions:

```ruby
Parity.configure do |config|
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're touching the README - where would this configuration go? Where does Parity look for configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've never actually needed to override these conventions. My understanding was that you added an initializer to your application, but now that I'm looking at the code I'm not sure that's happening (and furthermore wouldn't play nicely with the Travelling Ruby packaging/isolation that was recently introduced). @croaky, where does the configuration get set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this used to work in an initializer if you included parity in your Gemfile and built binstubs or your app, but doesn't apply if you are just using the generic Ruby commands or the Homebrew-installed version...

@geoffharcourt
Copy link
Collaborator Author

@gabebw, @croaky, @JoelQ, we've discussed this before, but now that we've removed app-specific config files, I am in favor of treating non-production databases as volatile and always sending the --confirm switch to Heroku.

@geoffharcourt geoffharcourt mentioned this pull request Sep 28, 2015
Closed
houndci-bot
houndci-bot reviewed Sep 28, 2015
View reviewed changes
spec/environment_spec.rb

expect(Parity::Backup).to have_received(:new).
with(
from: "production",
to: "staging",
additional_args: "--confirm myappname-staging"
additional_args: "--confirm parity-staging"
Copy link

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@geoffharcourt geoffharcourt force-pushed the bypass-confirmation branch 2 times, most recently from e2d60ff to d4f94ae Compare September 28, 2015 22:10
houndci-bot
houndci-bot reviewed Sep 28, 2015
View reviewed changes
spec/environment_spec.rb
expect(backup).to have_received(:restore)
end

it "passes arguments to the restore command when used against staging" do
it "passes the confirm argument when restoring to a non-production environment" do
Copy link

Choose a reason for hiding this comment

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

Line is too long. [85/80]

@geoffharcourt geoffharcourt force-pushed the bypass-confirmation branch 2 times, most recently from 7288060 to 2ac86a3 Compare September 29, 2015 02:00
@geoffharcourt
Allow bypass of confirmation when restoring to staging
d633d0c 
Heroku asks for confirmation with `--confirm appname` when performing
potentially destructive CLI operations such as a database restore. If
staging data is tracking with production or easily rebuilt using seeds,
this confirmation can be annoying, particularly during the common
operation of taking a snapshot of production and then immediately
copying it to staging to sync staging's DB up with production's.

This commit changes the behavior of non-production, non-local
development environments to append the confirmation argument on database
restores to overwrite the staging or other feature environment database
when restored from another environment.

Close thoughtbot#38.
JoelQ
JoelQ reviewed Sep 29, 2015
View reviewed changes
lib/parity/environment.rb
end

def restore_confirmation_argument
if !["production", "development"].include?(environment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why you added development to the protected list?

Also, thoughts on extracting this array to a constant? Maybe PROTECTED_ENVIRONMENTS ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Development will never need that argument since it's not a Heroku environment, but in the current organization of this code, #restore_confirmation_argument would never be called anyway. I'll change that.

@croaky has expressed a strong concern about overwriting production data by accident (hence why you can't restore to production), which I think is a good idea. Over a lot of time using parity I've come to the conclusion that anything outside production should be considered volatile and fair-game. An environment variable would be a great compromise if there's concern about blowing away staging data.

@geoffharcourt geoffharcourt added this to the 0.8.0 milestone Sep 29, 2015
@croaky
Copy link
Contributor

croaky commented Nov 6, 2015

Merged as ad2c21a. #feature #nice #doge

@croaky croaky closed this Nov 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
5 participants
@geoffharcourt @croaky @gabebw @JoelQ @houndci-bot