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

Be more explicit about the default of db:drop and db:create #13629

Merged
merged 1 commit into from Jan 8, 2014

Conversation

@dmathieu
Copy link
Contributor

@dmathieu dmathieu commented Jan 8, 2014

Closes #13625

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Jan 8, 2014

@dmathieu Can also add this to create task right? rake db:create also creates development and test databases by default right?

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Jan 8, 2014

You're right. Thank you. 🌞

@prathamesh-sonpatki
prathamesh-sonpatki reviewed Jan 8, 2014
View changes
activerecord/lib/active_record/railties/databases.rake Outdated
@@ -12,7 +12,7 @@ db_namespace = namespace :db do
end
end

desc 'Create the database from DATABASE_URL or config/database.yml for the current Rails.env (use db:create:all to create all databases in the config)'
desc 'Create the database from DATABASE_URL or config/database.yml for the current Rails.env (use db:create:all to create all databases in the config). Defaults to creating the development and test databases'
task :create => [:load_config] do

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 8, 2014
Member

@dmathieu I think it can be better worded by
Defaults to creating the development and test databases in absence of Rails.env

This comment has been minimized.

@dmathieu

dmathieu Jan 8, 2014
Author Contributor

There never is an absence of Rails.env. It defaults to development.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 8, 2014
Member

Oops. Yes you are right :)

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Jan 8, 2014

@dmathieu Will it work with [ci skip] ?

@senny
Copy link
Member

@senny senny commented Jan 8, 2014

From the description it's not obvious to me, when the default applies and how I can change it to only create the database of the current Rails.env.

@senny
Copy link
Member

@senny senny commented Jan 8, 2014

Looking closer I think we should also change the functionality. Currently:

  • bin/rake db:create => creates dev and test
  • RAILS_ENV=development bin/rake db:create => creates dev and test
  • RAILS_ENV=test bin/rake db:create => creates test

I think if the RAILS_ENV is specified it should only create the database for that env. This would also allow us to reword the description to:

Create the database from DATABASE_URL or config/database.yml 
for the current RAILS_ENV (use db:create:all to create all databases in the config). 
Without RAILS_ENV it defaults to creating the development and test databases.

This explains when the default is active and how it can be changed.

@senny
Copy link
Member

@senny senny commented Jan 8, 2014

This is even worse when using db:drop. For example:

RAILS_ENV=development bin/rake db:drop will also drop your test database.

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Jan 8, 2014

@senny: your idea definitely makes sense.
I just pushed it.

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/lib/active_record/tasks/database_tasks.rb Outdated
@@ -201,7 +201,7 @@ def class_for_adapter(adapter)

def each_current_configuration(environment)
environments = [environment]
environments << 'test' if environment == 'development'
environments << 'test' if environment == 'development' && ENV['RAILS_ENV'].nil?

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

this is quite hidden. Let's at least add a comment why that check is there.

This comment has been minimized.

@dmathieu

dmathieu Jan 8, 2014
Author Contributor

Done.

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/test/cases/tasks/database_tasks_test.rb Outdated
@@ -129,11 +129,22 @@ def test_creates_current_environment_database
)
end

def test_creates_test_database_when_environment_is_database
def test_creates_test_database_when_environment_is_nil

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

test names are really screwed up.

This comment has been minimized.

@dmathieu

dmathieu Jan 8, 2014
Author Contributor

Any opinion for a good name change?

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

let's say: test_creates_test_and_dev_databases_when_env_was_not_specified

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/test/cases/tasks/database_tasks_test.rb Outdated
)
end

def test_creates_test_database_when_environment_is_development

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

test names are really screwed up.

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/lib/active_record/tasks/database_tasks.rb Outdated
@@ -201,7 +201,8 @@ def class_for_adapter(adapter)

def each_current_configuration(environment)
environments = [environment]
environments << 'test' if environment == 'development'
# create the test database only if the environment hasn't been explicitely specified

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

as this method is not only used for creating we should reword it slightly:

# add test environment only if no RAILS_ENV was specified.
@senny
senny reviewed Jan 8, 2014
View changes
activerecord/test/cases/tasks/database_tasks_test.rb Outdated
)
end

def test_does_not_creates_test_database_when_rails_env_is_development

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

let's say: test_creates_only_dev_database_when_rails_env_is_development

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/test/cases/tasks/database_tasks_test.rb Outdated
@@ -239,11 +250,22 @@ def test_creates_current_environment_database
)
end

def test_creates_test_database_when_environment_is_database
def test_drops_test_database_when_environment_is_nil

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

lets say: test_drops_test_and_dev_databases_when_env_was_not_specified

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/test/cases/tasks/database_tasks_test.rb Outdated
)
end

def test_does_not_drop_test_database_when_environment_is_development

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

let's say: test_creates_only_dev_database_when_env_is_development

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/CHANGELOG.md Outdated
Previously, when the environment was development, we would automatically
create or drop both the test and development databases.

Now, if RAILS_ENV is explicitely defined at development, we don't create

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

typo: at => as

@senny
senny reviewed Jan 8, 2014
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,13 @@
* Don't create/drop the test database if we explicitely specified RAILS_ENV=development

Previously, when the environment was development, we would automatically

This comment has been minimized.

@senny

senny Jan 8, 2014
Member

let's replace automatically with always

senny added a commit that referenced this pull request Jan 8, 2014
Be more explicit about the default of db:drop and db:create
@senny senny merged commit b502e3d into rails:master Jan 8, 2014
@senny
Copy link
Member

@senny senny commented Jan 8, 2014

@dmathieu thanks 💛

@dmathieu dmathieu deleted the dmathieu:drop_test branch Jan 8, 2014
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.

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