Prevent destructive action on production database #22967

Merged
merged 7 commits into from Jan 8, 2016

Conversation

Projects
None yet
9 participants
@schneems
Member

schneems commented Jan 7, 2016

This PR introduces a key/value type store to Active Record that can be used for storing internal values. It is an alternative implementation to #21237 cc @sgrif @matthewd.

It is possible to run your tests against your production database by accident right now. While infrequently, but as an anecdotal data point, Heroku receives a non-trivial number of requests for a database restore due to this happening. In these cases the loss can be large.

To prevent against running tests against production we can store the "environment" version that was used when migrating the database in a new internal table. Before executing tests we can see if the database is a listed in protected_environments and abort. There is a manual escape valve to force this check from happening with environment variable DISABLE_DATABASE_ENVIRONMENT_CHECK=1.

+
+ def store(hash)
+ hash.each do |key, value|
+ first_or_initialize(key: key.to_s).update_attributes!(value: value)

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

Why the .to_s? AR's type casting should handle this.

@sgrif

sgrif Jan 7, 2016

Member

Why the .to_s? AR's type casting should handle this.

+ end
+
+ def value_for(key)
+ where(key: key.to_s).first.try(:value)

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

Could this be where(key: key).pluck(:value).first?

@sgrif

sgrif Jan 7, 2016

Member

Could this be where(key: key).pluck(:value).first?

+ end
+
+ def table_exists?
+ ActiveSupport::Deprecation.silence { connection.table_exists?(table_name) }

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

What are we silencing? Should we make sure we're deprecation free here?

@sgrif

sgrif Jan 7, 2016

Member

What are we silencing? Should we make sure we're deprecation free here?

+ connection.remove_index table_name, name: index_name
+ connection.drop_table(table_name)
+ end
+ end

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

It doesn't look like this method is ever used.

@sgrif

sgrif Jan 7, 2016

Member

It doesn't look like this method is ever used.

+ ActiveSupport::Deprecation.silence { connection.table_exists?(table_name) }
+ end
+
+ # Creates a schema table with columns +environment+ and +version+

This comment has been minimized.

@@ -1200,12 +1221,26 @@ def record_version_state_after_migrating(version)
if down?
migrated.delete(version)
ActiveRecord::SchemaMigration.where(:version => version.to_s).delete_all
- else
+ else

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

✂️

@sgrif

sgrif Jan 7, 2016

Member

✂️

+ hash.each do |key, value|
+ first_or_initialize(key: key.to_s).update_attributes!(value: value)
+ end
+ end

This comment has been minimized.

@sgrif

sgrif Jan 7, 2016

Member

Since this does one query per key, and it's only ever called with one key, what do you think about changing this to []= and value_for to []?

@sgrif

sgrif Jan 7, 2016

Member

Since this does one query per key, and it's only ever called with one key, what do you think about changing this to []= and value_for to []?

This comment has been minimized.

@matthewd

matthewd Jan 7, 2016

Member

👍

This comment has been minimized.

@schneems

schneems Jan 7, 2016

Member

I have mixed feelings about this. I don't want it to feel like hash access because it is hitting the database. I want to avoid "Oh, look a hash, hashes are fast. We can put lots of stuff in it" reaction. I also don't want people to accidentally try to use multiple level keys etc. It doesn't look like a hash because it isn't one. I can change if you like [] better though.

@schneems

schneems Jan 7, 2016

Member

I have mixed feelings about this. I don't want it to feel like hash access because it is hitting the database. I want to avoid "Oh, look a hash, hashes are fast. We can put lots of stuff in it" reaction. I also don't want people to accidentally try to use multiple level keys etc. It doesn't look like a hash because it isn't one. I can change if you like [] better though.

This comment has been minimized.

@matthewd

matthewd Jan 8, 2016

Member

That's fair. To me, absent a more comprehensive suite of hash-like methods, []/[]= are just the "right" way of spelling "get/set value using a key", though.

And this is so far away from anything a user would ever touch, it seems fine to go with whatever makes our code look neatest.

@matthewd

matthewd Jan 8, 2016

Member

That's fair. To me, absent a more comprehensive suite of hash-like methods, []/[]= are just the "right" way of spelling "get/set value using a key", though.

And this is so far away from anything a user would ever touch, it seems fine to go with whatever makes our code look neatest.

This comment has been minimized.

@sgrif

sgrif Jan 8, 2016

Member

Yeah, I think if this were public API that would be more of a concern. For
internal, I think [] and []= are fine.

That said, I don't feel particularly strongly on it, so I'll leave it up to
you since you did all the legwork on this.

On Thu, Jan 7, 2016 at 5:12 PM Matthew Draper notifications@github.com
wrote:

In activerecord/lib/active_record/internal_metadata.rb
#22967 (comment):

  •    "key"
    
  •  end
    
  •  def table_name
    
  •    "#{table_name_prefix}#{ActiveRecord::Base.internal_metadata_table_name}#{table_name_suffix}"
    
  •  end
    
  •  def index_name
    
  •    "#{table_name_prefix}unique_#{ActiveRecord::Base.internal_metadata_table_name}#{table_name_suffix}"
    
  •  end
    
  •  def store(hash)
    
  •    hash.each do |key, value|
    
  •      first_or_initialize(key: key.to_s).update_attributes!(value: value)
    
  •    end
    
  •  end
    

That's fair. To me, absent a more comprehensive suite of hash-like
methods, []/[]= are just the "right" way of spelling "get/set value using
a key", though.

And this is so far away from anything a user would ever touch, it seems
fine to go with whatever makes our code look neatest.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/22967/files#r49144499.

@sgrif

sgrif Jan 8, 2016

Member

Yeah, I think if this were public API that would be more of a concern. For
internal, I think [] and []= are fine.

That said, I don't feel particularly strongly on it, so I'll leave it up to
you since you did all the legwork on this.

On Thu, Jan 7, 2016 at 5:12 PM Matthew Draper notifications@github.com
wrote:

In activerecord/lib/active_record/internal_metadata.rb
#22967 (comment):

  •    "key"
    
  •  end
    
  •  def table_name
    
  •    "#{table_name_prefix}#{ActiveRecord::Base.internal_metadata_table_name}#{table_name_suffix}"
    
  •  end
    
  •  def index_name
    
  •    "#{table_name_prefix}unique_#{ActiveRecord::Base.internal_metadata_table_name}#{table_name_suffix}"
    
  •  end
    
  •  def store(hash)
    
  •    hash.each do |key, value|
    
  •      first_or_initialize(key: key.to_s).update_attributes!(value: value)
    
  •    end
    
  •  end
    

That's fair. To me, absent a more comprehensive suite of hash-like
methods, []/[]= are just the "right" way of spelling "get/set value using
a key", though.

And this is so far away from anything a user would ever touch, it seems
fine to go with whatever makes our code look neatest.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/22967/files#r49144499.

schneems added some commits Aug 14, 2015

Prevent destructive action on production database
This PR introduces a key/value type store to Active Record that can be used for storing internal values. It is an alternative implementation to #21237 cc @sgrif @matthewd.

It is possible to run your tests against your production database by accident right now. While infrequently, but as an anecdotal data point, Heroku receives a non-trivial number of requests for a database restore due to this happening. In these cases the loss can be large.

To prevent against running tests against production we can store the "environment" version that was used when migrating the database in a new internal table. Before executing tests we can see if the database is a listed in `protected_environments` and abort. There is a manual escape valve to force this check from happening with environment variable `DISABLE_DATABASE_ENVIRONMENT_CHECK=1`.
@sblackstone

This comment has been minimized.

Show comment
Hide comment
@sblackstone

sblackstone Jan 8, 2016

Could this not just be a key in database.yml per connection environment - rather than requiring this extra machinery..

Could this not just be a key in database.yml per connection environment - rather than requiring this extra machinery..

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 8, 2016

Owner

The problem is when someone say adds their production DB to their development credentials locally for diagnosing an issue then forgets they made the change and drops the DB.

Owner

schneems replied Jan 8, 2016

The problem is when someone say adds their production DB to their development credentials locally for diagnosing an issue then forgets they made the change and drops the DB.

@sblackstone

This comment has been minimized.

Show comment
Hide comment
@sblackstone

sblackstone Jan 8, 2016

Could this not just be a key in database.yml per connection environment - rather than requiring this extra machinery..

Could this not just be a key in database.yml per connection environment - rather than requiring this extra machinery..

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 8, 2016

Owner

The problem is when someone say adds their production DB to their development credentials locally for diagnosing an issue then forgets they made the change and drops the DB.

Owner

schneems replied Jan 8, 2016

The problem is when someone say adds their production DB to their development credentials locally for diagnosing an issue then forgets they made the change and drops the DB.

schneems added some commits Jan 7, 2016

Add EnvironmentMismatchError
Raise an error when a destructive action is made on a database where the current environment is different from the environment stored in the database.
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 8, 2016

Member

I added the environment mismatch error in activerecord/lib/active_record/tasks/database_tasks.rb we're only checking it on "protected" rake tasks. I also added [] and []= getter and setter methods

Member

schneems commented Jan 8, 2016

I added the environment mismatch error in activerecord/lib/active_record/tasks/database_tasks.rb we're only checking it on "protected" rake tasks. I also added [] and []= getter and setter methods

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 8, 2016

Member

All tests are 🍏 📗 💚

Member

schneems commented Jan 8, 2016

All tests are 🍏 📗 💚

sgrif added a commit that referenced this pull request Jan 8, 2016

Merge pull request #22967 from schneems/schneems/generic-metadata
Prevent destructive action on production database

@sgrif sgrif merged commit c1a1595 into rails:master Jan 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 8, 2016

Member

🚀

Member

schneems commented Jan 8, 2016

🚀

+ ActiveSupport::Deprecation.silence { connection.table_exists?(table_name) }
+ end
+
+ # Creates a internal metadata table with columns +key+ and +value+

This comment has been minimized.

@s-aida

s-aida Jan 9, 2016

Contributor

shouldn't it be an internal?

@s-aida

s-aida Jan 9, 2016

Contributor

shouldn't it be an internal?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 9, 2016

Member

Yes. Please send PR :)

@prathamesh-sonpatki

prathamesh-sonpatki Jan 9, 2016

Member

Yes. Please send PR :)

This comment has been minimized.

@s-aida

s-aida Jan 10, 2016

Contributor

See, but #22992 beats me.

@s-aida

s-aida Jan 10, 2016

Contributor

See, but #22992 beats me.

@kipcole9

This comment has been minimized.

Show comment
Hide comment
@kipcole9

kipcole9 Jan 9, 2016

When migrating an app to master branch with this PR applied, a destructive task detects there is no environment metadata and advises running rake db:migrate to fix.

However the environment is only stored after a migration is run. So if there are no pending migrations then the environment data is not added. This is because record_version_state_after_migrating which does the actual update is called only from within execute_migration_in_transaction below.

    def execute_migration_in_transaction(migration, direction)
      ddl_transaction(migration) do
        migration.migrate(direction)
        record_version_state_after_migrating(migration.version)
      end
    end

Therefore either the advise needs to change to an alternative strategy to update the environment metadata or rake db:migrate should alway update the environment metadata even when there are no pending migrations. One approach might be to modify the migrate method:

    def migrate
      if use_advisory_lock?
        with_advisory_lock { migrate_without_lock }
      else
        migrate_without_lock
      end
      ActiveRecord::InternalMetadata[:environment] = ActiveRecord::Migrator.current_environment
    end

and remove the update from record_version_state_after_migrating:

    def record_version_state_after_migrating(version)
      if down?
        migrated.delete(version)
        ActiveRecord::SchemaMigration.where(:version => version.to_s).delete_all
      else
        migrated << version
        ActiveRecord::SchemaMigration.create!(version: version.to_s)
      end
    end

However it also means this method name is not quite on target and probably should be renamed record_version_after_migration again.

kipcole9 commented Jan 9, 2016

When migrating an app to master branch with this PR applied, a destructive task detects there is no environment metadata and advises running rake db:migrate to fix.

However the environment is only stored after a migration is run. So if there are no pending migrations then the environment data is not added. This is because record_version_state_after_migrating which does the actual update is called only from within execute_migration_in_transaction below.

    def execute_migration_in_transaction(migration, direction)
      ddl_transaction(migration) do
        migration.migrate(direction)
        record_version_state_after_migrating(migration.version)
      end
    end

Therefore either the advise needs to change to an alternative strategy to update the environment metadata or rake db:migrate should alway update the environment metadata even when there are no pending migrations. One approach might be to modify the migrate method:

    def migrate
      if use_advisory_lock?
        with_advisory_lock { migrate_without_lock }
      else
        migrate_without_lock
      end
      ActiveRecord::InternalMetadata[:environment] = ActiveRecord::Migrator.current_environment
    end

and remove the update from record_version_state_after_migrating:

    def record_version_state_after_migrating(version)
      if down?
        migrated.delete(version)
        ActiveRecord::SchemaMigration.where(:version => version.to_s).delete_all
      else
        migrated << version
        ActiveRecord::SchemaMigration.create!(version: version.to_s)
      end
    end

However it also means this method name is not quite on target and probably should be renamed record_version_after_migration again.

@kipcole9

This comment has been minimized.

Show comment
Hide comment
@kipcole9

kipcole9 Jan 9, 2016

db:migrate without any pending migrations will not update the metadata store with the environment key since this line is only executed from execute_migration_in_transaction which will only happen if there are pending migrations.

Net result is that an application migrating to current master will not update the internal metadata using the advised rake db:migrate strategy

db:migrate without any pending migrations will not update the metadata store with the environment key since this line is only executed from execute_migration_in_transaction which will only happen if there are pending migrations.

Net result is that an application migrating to current master will not update the internal metadata using the advised rake db:migrate strategy

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 9, 2016

Owner

Good point, we will need to handle that case. Alternatively provide a rake task that only sets the environment.

Owner

schneems replied Jan 9, 2016

Good point, we will need to handle that case. Alternatively provide a rake task that only sets the environment.

schneems added a commit to schneems/rails that referenced this pull request Jan 11, 2016

Allow manually setting environment value
If for some reason some one is not able to set the environment from a migration this gives us an escape valve to manually set the environment for the database see rails#22967 (comment).

We will also fix the migration case, but this will ensure there is always a way to se the environment.

cc/ @sgrif

schneems added a commit to schneems/rails that referenced this pull request Jan 11, 2016

Allow manually setting environment value
If for some reason some one is not able to set the environment from a migration this gives us an escape valve to manually set the environment for the database see rails#22967 (comment).

We will also fix the migration case, but this will ensure there is always a way to set the environment.

cc/ @sgrif

schneems added a commit to schneems/rails that referenced this pull request Jan 11, 2016

Set environment even when no migration runs
This PR addresses the issue described in rails#22967 (comment). If the database is non empty and has no new migrations than `db:migrate` will not set the environment. This PR works by always setting the environment value on successful `up` migration regardless of whether or not a migration was actually executed.

pixeltrix added a commit that referenced this pull request Jan 23, 2016

Add environment back to db:structure:load
Because of the changes in #22967 the assumption in #18907 is no longer
true because the internal metadata feature for Active Record requires
a working environment.

@sgrif sgrif referenced this pull request in diesel-rs/diesel Jan 30, 2016

Closed

Add `diesel drop` to the CLI #146

@halilim

This comment has been minimized.

Show comment
Hide comment
@halilim

halilim Apr 3, 2016

Little late to the party and feeling like missing something obvious (sorry if this is the case), but shouldn't tests be allowed to run only on the test db? I can't think of a case I'd want to run tests on a non-test db and I also wouldn't like to accidentally destroy my development db by default for the sake of convenience.

So why not use a whitelist instead of a blacklist, e.g. allow_environments, or just remove it altogether?

halilim commented Apr 3, 2016

Little late to the party and feeling like missing something obvious (sorry if this is the case), but shouldn't tests be allowed to run only on the test db? I can't think of a case I'd want to run tests on a non-test db and I also wouldn't like to accidentally destroy my development db by default for the sake of convenience.

So why not use a whitelist instead of a blacklist, e.g. allow_environments, or just remove it altogether?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Apr 4, 2016

Member

The core idea behind this feature is to protect environments that are not recoverable. Maybe running tests against your development database might suck, but it won't be company or career ending. To that end we're using a blacklist, you are declaring the things you want to protect. If you REALLY don't want tests run against your development database, you could add that in to the protected environment.

You could always write your own whitelist

protected_environments = [Rails.env] unless UNPROTECTED_ENVIRONMENTS.include?(Rails.env)
Member

schneems commented Apr 4, 2016

The core idea behind this feature is to protect environments that are not recoverable. Maybe running tests against your development database might suck, but it won't be company or career ending. To that end we're using a blacklist, you are declaring the things you want to protect. If you REALLY don't want tests run against your development database, you could add that in to the protected environment.

You could always write your own whitelist

protected_environments = [Rails.env] unless UNPROTECTED_ENVIRONMENTS.include?(Rails.env)
@halilim

This comment has been minimized.

Show comment
Hide comment
@halilim

halilim Apr 6, 2016

@schneems I thought unprotected_environments would be a better default by requiring less modifications for the common case. Thanks for answering.

halilim commented Apr 6, 2016

@schneems I thought unprotected_environments would be a better default by requiring less modifications for the common case. Thanks for answering.

schneems pushed a commit that referenced this pull request Apr 6, 2016

Prevent db:schema:load to protected environments
Follow up to #22967 to protect against
loading a schema on accident in production.

cc @schneems

ohrite added a commit to minifast/parallel_tests that referenced this pull request Aug 5, 2016

Add integration test for Rails 5.0.0
* Fixes grosser#517
* Adds DISABLE_DATABASE_ENVIRONMENT_CHECK=1 as mentioned in rails/rails#22967

@ohrite ohrite referenced this pull request in grosser/parallel_tests Aug 5, 2016

Merged

Fixed: Rails 5.0 ActiveRecord::NoEnvironmentInSchemaError #519

ohrite added a commit to minifast/parallel_tests that referenced this pull request Aug 5, 2016

Add integration test for Rails 5.0.0
* Fixes grosser#517
* Adds DISABLE_DATABASE_ENVIRONMENT_CHECK=1 as mentioned in rails/rails#22967

ohrite added a commit to minifast/parallel_tests that referenced this pull request Aug 5, 2016

Add integration test for Rails 5.0.0
* Fixes grosser#517
* Adds DISABLE_DATABASE_ENVIRONMENT_CHECK=1 as mentioned in rails/rails#22967

ohrite added a commit to minifast/parallel_tests that referenced this pull request Aug 5, 2016

Add integration test for Rails 5.0.0
* Fixes grosser#517
* Adds DISABLE_DATABASE_ENVIRONMENT_CHECK=1 as mentioned in rails/rails#22967
@JanStevens

This comment has been minimized.

Show comment
Hide comment
@JanStevens

JanStevens Aug 9, 2016

Also pretty late to the party but stumbling on this issue I find the implementation a bit overkill for what the ultimate goal is: Prevent destructive rake tasks in production.

The main disadvantage for this MR is:

  • An extra internal table means that lots of gems will potentially brake (Apartment, SchemaPlus, to name a few. Let's not mention all the test runners).
  • Using migrations to magically find out the env that we are currently using, storing it in an internal table and then checking if it matches our current running env seems quite overkill.
  • There is no globally op out: I don't use heroku, lot's of people don't. I don't need this so why can't I op-out of the extra internal table?
  • Not documented: One has to sift through the github MR + issues it caused to find out what to do. There is also no warning about a possible breaking change mentioned about this feature! Lot's of gems now get bug reports that they don't work with rails 5. Making overall transition to rails 5 harder.
  • Recreating a test database with the combo RAILS_ENV=test rake db:drop db:create db:schema:load won't work. I need yet another rake task in the list, which makes no sense since I explicitly specify the env I'm running in.

For above reasons I propose the following alternative implementations:

  • A simple array protected_envs and then checking if current RAILS_ENV includes one of the protected_envs. As extra warning one could add a are you sure? confirmation.
  • Just a confirmation asking the user if he/she is sure when in a protected_env. I think that this would help 90% of the use cases (if there are any outside heroku)
  • Setting a required ENV variable before a destructive action can be taken place in a protected env
  • Implement this in the CLI of heroku, so we don't pollute rails core with yet another basecamp/heroku feature.

If my alternative implementations won't do or I'm missing a very obvious part then please enlighten me.

Also pretty late to the party but stumbling on this issue I find the implementation a bit overkill for what the ultimate goal is: Prevent destructive rake tasks in production.

The main disadvantage for this MR is:

  • An extra internal table means that lots of gems will potentially brake (Apartment, SchemaPlus, to name a few. Let's not mention all the test runners).
  • Using migrations to magically find out the env that we are currently using, storing it in an internal table and then checking if it matches our current running env seems quite overkill.
  • There is no globally op out: I don't use heroku, lot's of people don't. I don't need this so why can't I op-out of the extra internal table?
  • Not documented: One has to sift through the github MR + issues it caused to find out what to do. There is also no warning about a possible breaking change mentioned about this feature! Lot's of gems now get bug reports that they don't work with rails 5. Making overall transition to rails 5 harder.
  • Recreating a test database with the combo RAILS_ENV=test rake db:drop db:create db:schema:load won't work. I need yet another rake task in the list, which makes no sense since I explicitly specify the env I'm running in.

For above reasons I propose the following alternative implementations:

  • A simple array protected_envs and then checking if current RAILS_ENV includes one of the protected_envs. As extra warning one could add a are you sure? confirmation.
  • Just a confirmation asking the user if he/she is sure when in a protected_env. I think that this would help 90% of the use cases (if there are any outside heroku)
  • Setting a required ENV variable before a destructive action can be taken place in a protected env
  • Implement this in the CLI of heroku, so we don't pollute rails core with yet another basecamp/heroku feature.

If my alternative implementations won't do or I'm missing a very obvious part then please enlighten me.

@rails rails locked and limited conversation to collaborators Aug 9, 2016

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Aug 15, 2016

Member

This feature is already in Rails. It's not coming out unless there's a really good reason. If you really want you can fork active record and remove it. There is already extensive conversation around alternative proposals and why they will or won't work.

An extra internal table means that lots of gems will potentially brake (Apartment, SchemaPlus, to name a few. Let's not mention all the test runners).

Open an issue with those gems or if there's a problem open an issue on Rails.

Using migrations to magically find out the env that we are currently using, storing it in an internal table and then checking if it matches our current running env seems quite overkill.

No, we talked about it above, it's the only viable solution.

There is no globally op out: I don't use heroku, lot's of people don't. I don't need this so why can't I op-out of the extra internal table?

That's not correct there's an env var you can set to allow you to disable the check. If you want to opt out of even having the table, you can fork active record.

Not documented: One has to sift through the github MR + issues it caused to find out what to do.

This is where you can help. What were you trying to do when you hit a problem? This feature is documented, and claiming it isn't doesn't help us know what failed or why or when. The best docs come from people who use the product. Either open an issue with your failure case or submit a doc PR (an easy way to get on that leader board).

There is also no warning about a possible breaking change mentioned about this feature!

You're bumping major versions of Rails, which has lots of breaking changes. How exactly would you have like to be notified about this new feature as a breaking change?

Lot's of gems now get bug reports that they don't work with rails 5. Making overall transition to rails 5 harder.

Again, specifics. We need specifics.

Recreating a test database with the combo RAILS_ENV=test rake db:drop db:create db:schema:load won't work. I need yet another rake task in the list, which makes no sense since I explicitly specify the env I'm running in.

This is a really good piece of data. I don't use Rails like that, but you do. If you don't open an issue about these things we won't know about them. This really valuable piece of data is burried in your ranty comment on this pull request. I didn't like your tone, so honestly I didn't even read it until today.

If you want to make progress here, please open an issue with that as the text. You can even include example output:

$ RAILS_ENV=test rake db:drop db:create db:schema:load
DEPRECATION WARNING: `config.serve_static_files` is deprecated and will be removed in Rails 5.1.
Please use `config.public_file_server.enabled = true` instead.
 (called from block in <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environments/test.rb:16)
DEPRECATION WARNING: `config.static_cache_control` is deprecated and will be removed in Rails 5.1.
Please use
`config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }`
instead.
 (called from block in <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environments/test.rb:17)
DEPRECATION WARNING: ActiveRecord::Base.raise_in_transactional_callbacks= is deprecated, has no effect and will be removed without replacement. (called from <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environment.rb:5)
rake aborted!
ActiveRecord::NoEnvironmentInSchemaError:

Environment data not found in the schema. To resolve this issue, run:

A simple array protected_envs and then checking if current RAILS_ENV includes one of the protected_envs. As extra warning one could add a are you sure? confirmation.

Won't work. Also confirmations break automated work flows and scripts.

Just a confirmation asking the user if he/she is sure when in a protected_env. I think that this would help 90% of the use cases (if there are any outside heroku)

Confirmations break automated work flows and scripts. We essentially have this by allowing you to manage your experience via environment variables. We even list how to proceed if you really want to in the error message

Environment data not found in the schema. To resolve this issue, run:

    bin/rails db:environment:set RAILS_ENV=test

Setting a required ENV variable before a destructive action can be taken place in a protected env

Won't work.

Implement this in the CLI of heroku, so we don't pollute rails core with yet another basecamp/heroku feature.

The problem does not only exist on Heroku it exists anywhere a dev can drop a database, like when one of your co-workers copys production credentials to their local machine and forgets about it before accidentally doing a schema reset or something. Yes it's rare, but the cost to prevent it (this implementation) is pretty insubstantial to the cost of losing your whole production database.

This isn't a "Heroku" feature, it is a Rails feature.

Member

schneems commented Aug 15, 2016

This feature is already in Rails. It's not coming out unless there's a really good reason. If you really want you can fork active record and remove it. There is already extensive conversation around alternative proposals and why they will or won't work.

An extra internal table means that lots of gems will potentially brake (Apartment, SchemaPlus, to name a few. Let's not mention all the test runners).

Open an issue with those gems or if there's a problem open an issue on Rails.

Using migrations to magically find out the env that we are currently using, storing it in an internal table and then checking if it matches our current running env seems quite overkill.

No, we talked about it above, it's the only viable solution.

There is no globally op out: I don't use heroku, lot's of people don't. I don't need this so why can't I op-out of the extra internal table?

That's not correct there's an env var you can set to allow you to disable the check. If you want to opt out of even having the table, you can fork active record.

Not documented: One has to sift through the github MR + issues it caused to find out what to do.

This is where you can help. What were you trying to do when you hit a problem? This feature is documented, and claiming it isn't doesn't help us know what failed or why or when. The best docs come from people who use the product. Either open an issue with your failure case or submit a doc PR (an easy way to get on that leader board).

There is also no warning about a possible breaking change mentioned about this feature!

You're bumping major versions of Rails, which has lots of breaking changes. How exactly would you have like to be notified about this new feature as a breaking change?

Lot's of gems now get bug reports that they don't work with rails 5. Making overall transition to rails 5 harder.

Again, specifics. We need specifics.

Recreating a test database with the combo RAILS_ENV=test rake db:drop db:create db:schema:load won't work. I need yet another rake task in the list, which makes no sense since I explicitly specify the env I'm running in.

This is a really good piece of data. I don't use Rails like that, but you do. If you don't open an issue about these things we won't know about them. This really valuable piece of data is burried in your ranty comment on this pull request. I didn't like your tone, so honestly I didn't even read it until today.

If you want to make progress here, please open an issue with that as the text. You can even include example output:

$ RAILS_ENV=test rake db:drop db:create db:schema:load
DEPRECATION WARNING: `config.serve_static_files` is deprecated and will be removed in Rails 5.1.
Please use `config.public_file_server.enabled = true` instead.
 (called from block in <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environments/test.rb:16)
DEPRECATION WARNING: `config.static_cache_control` is deprecated and will be removed in Rails 5.1.
Please use
`config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }`
instead.
 (called from block in <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environments/test.rb:17)
DEPRECATION WARNING: ActiveRecord::Base.raise_in_transactional_callbacks= is deprecated, has no effect and will be removed without replacement. (called from <top (required)> at /Users/richardschneeman/Documents/projects/tmp/loggertest/config/environment.rb:5)
rake aborted!
ActiveRecord::NoEnvironmentInSchemaError:

Environment data not found in the schema. To resolve this issue, run:

A simple array protected_envs and then checking if current RAILS_ENV includes one of the protected_envs. As extra warning one could add a are you sure? confirmation.

Won't work. Also confirmations break automated work flows and scripts.

Just a confirmation asking the user if he/she is sure when in a protected_env. I think that this would help 90% of the use cases (if there are any outside heroku)

Confirmations break automated work flows and scripts. We essentially have this by allowing you to manage your experience via environment variables. We even list how to proceed if you really want to in the error message

Environment data not found in the schema. To resolve this issue, run:

    bin/rails db:environment:set RAILS_ENV=test

Setting a required ENV variable before a destructive action can be taken place in a protected env

Won't work.

Implement this in the CLI of heroku, so we don't pollute rails core with yet another basecamp/heroku feature.

The problem does not only exist on Heroku it exists anywhere a dev can drop a database, like when one of your co-workers copys production credentials to their local machine and forgets about it before accidentally doing a schema reset or something. Yes it's rare, but the cost to prevent it (this implementation) is pretty insubstantial to the cost of losing your whole production database.

This isn't a "Heroku" feature, it is a Rails feature.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Aug 15, 2016

Member

I appreciate your desire to be involved with the process. I'm frustrated by the amount of time i'm having to re-explain myself over the mailing list and github comments over this issue. I just got back from vacation and would much rather spend time fixing bugs, or writing new features, or cutting releases of sprockets rather than waste so much time on github comments.

I've pointed out some productive avenues for your experiences. I really appreciate you bringing the failure case to our attention. You can open a new issue and mention be @schneems and I can take a look at it later.

Member

schneems commented Aug 15, 2016

I appreciate your desire to be involved with the process. I'm frustrated by the amount of time i'm having to re-explain myself over the mailing list and github comments over this issue. I just got back from vacation and would much rather spend time fixing bugs, or writing new features, or cutting releases of sprockets rather than waste so much time on github comments.

I've pointed out some productive avenues for your experiences. I really appreciate you bringing the failure case to our attention. You can open a new issue and mention be @schneems and I can take a look at it later.

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