Add database config option `database_tasks: false` by westonganger · Pull Reques... - JOYK Joy of Geek, Geek News, Link all geek

Conversation

Copy link

Contributor

westonganger commented on Jul 15

Attempting to solve #42787

WIP. This is probably a reasonable start. Need help with testing. Please let me know if anyone wants to pick up the torch or help out.

I believe the tests for this need to be added within activerecord/test/cases/tasks/database_tasks_test.rb though im not sure exactly how to go about this.

activerecord/lib/active_record/database_configurations/hash_config.rb

Outdated

@@ -103,6 +103,14 @@ def adapter

def schema_cache_path

configuration_hash[:schema_cache_path]

end

def database_tasks

ghiculescu on Jul 15

Member

I think this would give a better idea of the method's purpose:

Suggested change

def database_tasks def database_tasks?

eileencodes on Jul 16

Member

Also I think this should match the other tasks and use fetch and include replica since those dont run tasks either: Ex: configuration_hash.fetch(:database_tasks, true) || replica?

westonganger on Jul 16

Author

Contributor

Ok I have made it database_tasks? and changed the method to use replica and fetch. Doesnt look exactly as you described it though, but I think its correct.

Copy link

Member

eileencodes left a comment

There's one other place that needs to be updated - the configs_for method isn't taking the database tasks into account and needs to be updated as well.

activerecord/lib/active_record/tasks/database_tasks.rb

Outdated

if db_config.database_tasks

yield db_config.name

else

next ### Skip registering and using this DB's tasks

eileencodes on Jul 16

Member

I'd prefer an early return here like this:

database_configs.each do |db_config|
  next unless db_config.database_tasks

  yield db_config.name
end

westonganger on Jul 16

Author

Contributor

Updated.

activerecord/lib/active_record/database_configurations/hash_config.rb

Outdated

@@ -103,6 +103,14 @@ def adapter

def schema_cache_path

configuration_hash[:schema_cache_path]

end

def database_tasks

eileencodes on Jul 16

Member

Also I think this should match the other tasks and use fetch and include replica since those dont run tasks either: Ex: configuration_hash.fetch(:database_tasks, true) || replica?

guides/source/active_record_multiple_databases.md

Outdated

@@ -196,6 +196,21 @@ Note that there is no command for creating the database users, and you'll need t

to support the readonly users for your replicas. If you want to create just the animals

database you can run `bin/rails db:create:animals`.

## Connecting to Databases without Managing Schema and Migrations

If you would like to connect to an external database without any database mangement tasks such as schema management, migrations, seeds, etc. you can use the per database config option `database_tasks: false` by default it is considered true.

eileencodes on Jul 16

Member

Can you wrap this documentation to 80 characters?

westonganger on Jul 16

Author

Contributor

Done. Seemed like the rest of the file used ~100 though.

westonganger

added a commit to westonganger/rails that referenced this pull request

on Jul 16

Copy link

Contributor

Author

westonganger commented on Jul 16

Ok thanks for the tip regarding configs_for I have now updated that along with the other commented items.

Copy link

Member

eileencodes left a comment

I left some comments. This also needs a CHANGELOG entry in Active Record. This also needs a test confirming that tasks aren't run on configs with database_tasks: false (for example on db:migrate). You can add a test for that in railties/test//application/rake/multi_dbs_test.rb.

activerecord/test/cases/database_configurations/hash_config_test.rb

Outdated

def test_database_tasks_defaults_to_true

config = HashConfig.new("default_env", "primary", {})

assert_equal config.database_tasks, true

eileencodes on Jul 20

Member

These asserts are backwards they should be written as assert_equal true, config.database_tasks.

activerecord/test/cases/database_configurations/hash_config_test.rb

Outdated

config = HashConfig.new("default_env", "primary", database_tasks: false)

assert_equal config.database_tasks, false

config = HashConfig.new("default_env", "primary", database_tasks: 'str')

eileencodes on Jul 20

Member

This should use double quotes.

Suggested change

config = HashConfig.new("default_env", "primary", database_tasks: 'str') config = HashConfig.new("default_env", "primary", database_tasks: "str")

guides/source/active_record_multiple_databases.md

Outdated

```yaml

production:

primary:

database: my_animals_database

eileencodes on Jul 20

Member

primary and animals shouldn't have the same db name.

Suggested change

database: my_animals_database database: my_database

activerecord/lib/active_record/database_configurations/hash_config.rb

Outdated

@@ -103,6 +103,10 @@ def adapter

def schema_cache_path

configuration_hash[:schema_cache_path]

end

def database_tasks?

eileencodes on Jul 20

Member

Suggested change

def database_tasks? def database_tasks? # :nodoc:

guides/source/active_record_multiple_databases.md

Outdated

If you would like to connect to an external database without any database

mangement tasks such as schema management, migrations, seeds, etc. you can use

the per database config option `database_tasks: false` by default it is

eileencodes on Jul 20

Member

Suggested change

the per database config option `database_tasks: false` by default it is the per database config option `database_tasks: false`. By default it is

guides/source/active_record_multiple_databases.md

Outdated

If you would like to connect to an external database without any database

mangement tasks such as schema management, migrations, seeds, etc. you can use

the per database config option `database_tasks: false` by default it is

considered true.

eileencodes on Jul 20

Member

Suggested change

considered true. set to true.

guides/source/active_record_multiple_databases.md

Outdated

## Connecting to Databases without Managing Schema and Migrations

If you would like to connect to an external database without any database

mangement tasks such as schema management, migrations, seeds, etc. you can use

eileencodes on Jul 20

Member

Suggested change

mangement tasks such as schema management, migrations, seeds, etc. you can use mangement tasks such as schema management, migrations, seeds, etc. you can set

westonganger

force-pushed the

westonganger:multi_db_config_database_tasks_option

branch from 77a4bcb to 2b9ecd0

on Jul 23

westonganger

force-pushed the

westonganger:multi_db_config_database_tasks_option

branch 5 times, most recently from dc7ddd2 to 8d9bb21

9 days ago

Copy link

Contributor

Author

westonganger commented 9 days ago

@eileencodes I have made the requested changes.

I have also added a better test, however I am getting an test failure that I am not expecting. Maybe my check for database existence is not appropriate?

Its currently failing on Line 116

test "when database_tasks is false, then do not run the database tasks on that db" do app_file "config/database.yml", <<-YAML development: primary: database: db/default.sqlite3 adapter: sqlite3 animals: database: db/development_animals.sqlite3 adapter: sqlite3 database_tasks: false schema_dump: true ### database_tasks should override all sub-settings YAML Dir.chdir(app_path) do primary_db_exists = lambda{ rails("runner", "puts !!(ActiveRecord::Base.connection rescue false)").strip } animals_db_exists = lambda{ rails("runner", "puts !!(AnimalsBase.connection rescue false)").strip } generate_models_for_animals assert_raise do rails "db:drop:animals" end assert_equal "true", animals_db_exists.call assert_raise do rails "db:create:animals" end assert_raise do rails "db:migrate:animals" end assert_raise do rails "db:schema:load:animals" end assert_raise do rails "db:schema:dump:animals" end rails "db:schema:dump" assert_not File.exist?("db/animals_schema.yml") assert_not_raise do rails "db:drop" end assert_equal "true", animals_db_exists.call assert_equal "false", primary_db_exists.call assert_not_raise do rails "db:create" rails "db:schema:load" rails "db:migrate" end assert File.exist?("db/schema.rb") assert_not File.exist?("db/animals_schema.rb") end end

Copy link

Member

eileencodes commented 5 days ago

Checking for the connection isn't going to be a good way to test whether the db exists because ActiveRecord::Base.connection will always exist, it's the default. Since you are really only testing that the tasks are skipped for animals, you can remove the connection checks for the primary. I also think the test can be simplified a bit - we don't need to test every rake task, just a few.

westonganger

force-pushed the

westonganger:multi_db_config_database_tasks_option

branch from c8ce5dd to a77dd10

5 days ago

westonganger

changed the title [WIP] Add database config option database_tasks: false

Add database config option database_tasks: false

5 days ago

Copy link

Contributor

Author