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

Add config to disable schema dump after migration #13948

Merged
merged 1 commit into from Feb 6, 2014

Conversation

@emilsoman
Copy link
Contributor

@emilsoman emilsoman commented Feb 5, 2014

  • Add a config on active_record named dump_schema_after_migration
  • Schema dump doesn't happen if the config is set to false
  • Set default value of the config to true
  • Set config in generated production and test environment files to false
  • Update configuration guide
  • Update CHANGELOG
@senny senny added the activerecord label Feb 5, 2014
@senny
Copy link
Member

@senny senny commented Feb 5, 2014

@@ -1,3 +1,12 @@
* Add flag to disable schema dump after migration

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

missing . at the end.

Add a config on active_record named `dump_schema_after_migration`
which is true by default. Now schema dump does not happen at the
end of migration rake task if `dump_schema_after_migration` is set
to false.

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

let's use aroundtrueandfalse`.

# Specify whether schema dump should happen at the end of
# db:migrate rake task. This is true by default, which is useful for
# development environment. This should ideally be false in production
# environment where dumping schema is rarely needed.

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

let's use +true+ and +false+.

@@ -292,6 +292,8 @@ All these configuration options are delegated to the `I18n` library.

* `config.active_record.maintain_test_schema` is a boolean value which controls whether Active Record should try to keep your test database schema up-to-date with `db/schema.rb` (or `db/structure.sql`) when you run your tests. The default is true.

* `config.active_record.dump_schema_after_migration` is a boolean value which controls whether or not schema dump should happen ( `db/schema.rb` or `db/structure.sql`) when you run migrations. This is set to false in `config/environments/production.rb` and `config/environments/test.rb` which are generated by Rails. The default value is true if this configuration is not set.

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

can you wrap this line to 80 chars?

This comment has been minimized.

@emilsoman

emilsoman Feb 5, 2014
Author Contributor

The rest of the configurations are documented as single long lines. If I wrap the description of this config alone , I'm not sure if it will screw up the readability of the file as a whole. What do you think ?

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

It won't affect the readability because the word wraps do not change the rendered html. Some time ago we decided to move towards 80 char lines in the source files for readability. We didn't change the word wrap of existing documents to keep the history but for additions and modifications we look to keep it to 80 chars. Basically a slow migration ;).

@@ -81,4 +81,9 @@ Rails.application.configure do

# Use default logging formatter so that PID and timestamp are not suppressed.
config.log_formatter = ::Logger::Formatter.new

<%- unless options.skip_active_record? -%>
# Do not dump schema after migrations

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

let's end with a ..

@@ -36,4 +36,9 @@ Rails.application.configure do

# Raises error for missing translations
# config.action_view.raise_on_missing_translations = true

<%- unless options.skip_active_record? -%>
# Do not dump schema after migrations

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

let's end with a ..

assert_match(/create_table \"books\"/, structure_dump)
end
end

This comment has been minimized.

@senny

senny Feb 5, 2014
Member

✂️ (only the blank line)

@senny
Copy link
Member

@senny senny commented Feb 5, 2014

@emilsoman this is looking good. I added a few minor comments about formatting and style.

/cc @fxn

@senny senny closed this Feb 5, 2014
@senny senny reopened this Feb 5, 2014
@@ -81,4 +81,9 @@ Rails.application.configure do

# Use default logging formatter so that PID and timestamp are not suppressed.
config.log_formatter = ::Logger::Formatter.new

<%- unless options.skip_active_record? -%>

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 5, 2014
Member

You should move the blank line to inside the unless clause, so that it doesn't end up with extra blank line when skipping AR.

@@ -36,4 +36,9 @@ Rails.application.configure do

# Raises error for missing translations
# config.action_view.raise_on_missing_translations = true

<%- unless options.skip_active_record? -%>

This comment has been minimized.

@emilsoman
Copy link
Contributor Author

@emilsoman emilsoman commented Feb 5, 2014

@senny , @carlosantoniodasilva Thanks for the feedback ! Fixed formatting and style issues. Anything else ?

@fxn
Copy link
Member

@fxn fxn commented Feb 5, 2014

Hey, gonna have a look today.

@senny
Copy link
Member

@senny senny commented Feb 5, 2014

@emilsoman you will need to squash all your commits into a single one but let's wait for feedback from @fxn first.

@fxn
Copy link
Member

@fxn fxn commented Feb 5, 2014

Looks good. Just a couple of details.

I believe the flag should not appear in test.rb. I explained the rationale in the mailing list but the summary is that since you are not supposed to run migrations in the test environment, it looks strange that the config file says anything about them. The comment is enlightening, see "Do not dump schema after migrations", "in the test environment? which migrations? I am not supposed to run migrations in the test environment!", the user may wonder.

Since migrations do not run, the implicit true value is fine, it won't hurt.

Then some details regarding the values of the flag. We try to avoid using singletons in docs and tests. As far as the user is concerned this is a flag. We could assign anything to it that is true, and the user can set it to nil if he so wishes, we only use the flag for its boolean interpretation. I am going to do some inline comments to be more specific.

@@ -1,3 +1,12 @@
* Add flag to disable schema dump after migration.

Add a config on active_record named `dump_schema_after_migration`

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

config parameter

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

I know what you mean by active_record, but by itself doesn't have a lot of meaning. Better to just say "Active Record", those config parameters configure the ORM, "Active Record" is going to be good.

* Add flag to disable schema dump after migration.

Add a config on active_record named `dump_schema_after_migration`
which is `true` by default. Now schema dump does not happen at the

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

regular font for "true".

This comment has been minimized.

@emilsoman

emilsoman Feb 6, 2014
Author Contributor

Changed to true after @senny commented. /cc @senny

Add a config on active_record named `dump_schema_after_migration`
which is `true` by default. Now schema dump does not happen at the
end of migration rake task if `dump_schema_after_migration` is set
to `false`.

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

just "is false" (instead of "set to false")

@@ -76,6 +76,15 @@ def self.configurations
mattr_accessor :timestamped_migrations, instance_writer: false
self.timestamped_migrations = true

##
# :singleton-method:
# Specify whether schema dump should happen at the end of

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

at the end of the db:migrate task.

##
# :singleton-method:
# Specify whether schema dump should happen at the end of
# db:migrate rake task. This is +true+ by default, which is useful for

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

regular font for "true"

# :singleton-method:
# Specify whether schema dump should happen at the end of
# db:migrate rake task. This is +true+ by default, which is useful for
# development environment. This should ideally be +false+ in production

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

regular font for "false"

@@ -292,6 +292,13 @@ All these configuration options are delegated to the `I18n` library.

* `config.active_record.maintain_test_schema` is a boolean value which controls whether Active Record should try to keep your test database schema up-to-date with `db/schema.rb` (or `db/structure.sql`) when you run your tests. The default is true.

* `config.active_record.dump_schema_after_migration` is a boolean value which

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

"boolean value" could be confusing, in Ruby there is no Boolean class, and all values are boolean values. We could say here "is a flag".


require "#{app_path}/config/environment"

assert_equal false, ActiveRecord::Base.dump_schema_after_migration

This comment has been minimized.

@fxn

fxn Feb 5, 2014
Member

No need to commit to a particular value, testing with a bare "assert" is enough. This applies to the other tests as well.

This comment has been minimized.

@emilsoman

emilsoman Feb 6, 2014
Author Contributor

This test is to make sure that the generated config/environments/production.rb has config.active_record.dump_schema_after_migration set to false. Should I rather be using refute ActiveRecord::Base.dump_schema_after_migration ? Is that what you meant ? Sorry I didn't get what you meant by 'commit to a particular value'.

This comment has been minimized.

@senny

senny Feb 6, 2014
Member

@emilsoman the idea is, that we should not care if it's actually false or just falsy. So instead of assert_equal false, X just assert_not X. This means we don't commit to the particular value of false.

@emilsoman
Copy link
Contributor Author

@emilsoman emilsoman commented Feb 6, 2014

@fxn , Thanks for the amazing feedback ! I've removed the config from config/environments/test.rb because what you said makes perfect sense. I've fixed the grammatical issues that you pointed out. I am not sure about a couple of things for which I've left inline comments. Please take a look whenever you can. Thank you :)

@senny
Copy link
Member

@senny senny commented Feb 6, 2014

@emilsoman sorry for pointing you in the wrong direction with true and false 😓

@fxn
Copy link
Member

@fxn fxn commented Feb 6, 2014

Exactly :), the idea is that we want to communicate that the default config is such that the schema is not dumped in production. The exact value needed to enable or disable the flag doesn't matter, any value works (interpreted as a flag).

@fxn
fxn reviewed Feb 6, 2014
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
* Add flag to disable schema dump after migration.

Add a config parameter on ActiveRecord named `dump_schema_after_migration`

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

Please note that "Active Record" has a space in its name, Active Record is an ORM, and ActiveRecord is a Ruby module that generally has little to have documented.

@fxn
fxn reviewed Feb 6, 2014
View changes
activerecord/lib/active_record/core.rb Outdated
##
# :singleton-method:
# Specify whether schema dump should happen at the end of the
# db:migrate rake task. This is true by default, which is useful for

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

for the development environment

@fxn
fxn reviewed Feb 6, 2014
View changes
activerecord/lib/active_record/core.rb Outdated
# :singleton-method:
# Specify whether schema dump should happen at the end of the
# db:migrate rake task. This is true by default, which is useful for
# development environment. This should ideally be false in production

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

in the production environment

@fxn
fxn reviewed Feb 6, 2014
View changes
guides/source/configuring.md Outdated
@@ -292,6 +292,12 @@ All these configuration options are delegated to the `I18n` library.

* `config.active_record.maintain_test_schema` is a boolean value which controls whether Active Record should try to keep your test database schema up-to-date with `db/schema.rb` (or `db/structure.sql`) when you run your tests. The default is true.

* `config.active_record.dump_schema_after_migration` is a flag which
controls whether or not schema dump should happen ( `db/schema.rb` or

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

spurious space after the paren

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/CHANGELOG.md Outdated
@@ -1,3 +1,10 @@
* Set `dump_schema_after_migration` config values in production

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

period at the end of the sentence

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/CHANGELOG.md Outdated
* Set `dump_schema_after_migration` config values in production

Set `config.active_record.dump_schema_after_migration` as false
in generated `config/environments/production.rb` file

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

in the generated ..., also missing a period at the end

@fxn
Copy link
Member

@fxn fxn commented Feb 6, 2014

I made some editorial comments. Almost there!

@emilsoman
Copy link
Contributor Author

@emilsoman emilsoman commented Feb 6, 2014

@fxn Fixed. Can I squash the commits ?

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/test/application/configuration_test.rb Outdated
@@ -781,5 +781,22 @@ def index
assert_not Rails.configuration.respond_to?(:method_missing)
assert Rails.configuration.respond_to?(:method_missing, true)
end

test "config.active_record.dump_schema_after_migration is falsy on production" do

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

we try to avoid "truthy" and "falsy", "false" is enough

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/test/application/configuration_test.rb Outdated

require "#{app_path}/config/environment"

refute ActiveRecord::Base.dump_schema_after_migration

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

in general we prefer assert_not to refute, this is stylistic preference of the team

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/test/application/rake/migrations_test.rb Outdated
bundle exec rake db:migrate`

structure_dump = File.read("db/schema.rb")
assert_match(/create_table \"authors\"/, structure_dump)

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

quotes are not metacharacters in regexps, no need to escape them

@fxn
fxn reviewed Feb 6, 2014
View changes
railties/test/application/rake/migrations_test.rb Outdated
bundle exec rake db:migrate`

structure_dump = File.read("db/schema.rb")
assert_match(/create_table \"books\"/, structure_dump)

This comment has been minimized.

@fxn

fxn Feb 6, 2014
Member

same here

@fxn
Copy link
Member

@fxn fxn commented Feb 6, 2014

Awesome @emilsoman, added a few minor remarks and I think it is good to go. If you revise those and squash we'll apply.

* Add a config on Active Record named `dump_schema_after_migration`
* Schema dump doesn't happen if the config is set to false
* Set default value of the config to true
* Set config in generated production environment file to false
* Update configuration guide
* Update CHANGELOG
@emilsoman
Copy link
Contributor Author

@emilsoman emilsoman commented Feb 6, 2014

@fxn , done :)

@fxn
Copy link
Member

@fxn fxn commented Feb 6, 2014

Fantastic! Thanks a lot for working on this 😄 ❤️

@emilsoman
Copy link
Contributor Author

@emilsoman emilsoman commented Feb 6, 2014

Thanks for guiding this into good shape. You guys were super helpful. Thank you !

@fxn fxn merged commit 8806768 into rails:master Feb 6, 2014
@fxn
Copy link
Member

@fxn fxn commented Feb 6, 2014

Merged here.

@bronson

This comment has been minimized.

Copy link
Contributor

@bronson bronson commented on 8806768 Jan 19, 2016

If anyone else is wondering why this config was added: https://groups.google.com/forum/#!topic/rubyonrails-core/h4cQXmKuB7M

Dumping the production schema seems harmless enough to me. Then again, I've never had a database with enough tables where time is an issue.

@gkop
Copy link
Contributor

@gkop gkop commented Apr 6, 2018

Scott I agree it's generally harmless. It bit us though when we switched from schema to structure, as we rightfully didn't supply pg_dump in our production environments. So grateful for Emil's contribution :)

neerajdotname added a commit to bigbinary/wheel that referenced this pull request Mar 13, 2020
* Updated sample database.yml file.

* Added sidekiq config file.

* Removed rails_12factor gem.

Added required config changes for heroku logging.

* Removed tagged_logging initializer file.

The tagged logging configs are handled separately for each env.

* Do not dump schema after migrations.

https://groups.google.com/forum/#!topic/rubyonrails-core/h4cQXmKuB7M
rails/rails#13948

* Enabled force_ssl config.

* Added comments about config/initializers/assets.rb.

* reverting some of the changes

Co-authored-by: Raj Singh <raj.singh@vineti.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.