-
Notifications
You must be signed in to change notification settings - Fork 500
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 support for multiple DBs in Rails >= 6.1 #930
Add support for multiple DBs in Rails >= 6.1 #930
Conversation
lib/parallel_tests/tasks.rb
Outdated
return unless configured_databases.present? && block_given? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just remove this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the configured_databases.present?
piece is fine since it'll just be empty otherwise 😄 Since this is an internal API, if you're comfortable not having the safety of block_given?
then I can yoink that out as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/parallel_tests/tasks.rb
Outdated
# drop individual databases in multi DB setups | ||
ParallelTests::Tasks.for_each_database do |name| | ||
desc "Drop test #{name} database via db:drop:#{name} --> parallel:drop:#{name}[num_cpus]" | ||
task "drop:#{name}".to_sym, :count do |_, args| | ||
ParallelTests::Tasks.run_in_parallel( | ||
[ | ||
$0, | ||
"db:drop:#{name}", | ||
"RAILS_ENV=#{ParallelTests::Tasks.rails_env}", | ||
"DISABLE_DATABASE_ENVIRONMENT_CHECK=1" | ||
], | ||
args | ||
) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we reduce the copy paste by making for_each_database also return nil
for the base db ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear, for this example db:drop
drops all databases, not the primary
/default DB in a multi-DB setup. But I can definitely adjust this a bit so we don't need two separate blocks to define the "drop all" and the "drop individual" tasks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds good :)
nice! |
looks like it's needs a |
@@ -135,6 +135,8 @@ def configured_databases | |||
end | |||
|
|||
def for_each_database(&block) | |||
return unless defined?(ActiveRecord) | |||
|
|||
# Use nil to represent all databases | |||
block&.call(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be after this to preserve original behavior ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh gosh, nice catch 😄 0f8e6a8
FYI cleanup |
4.5.0 |
Added support for performing
create
,migrate
,drop
, andload_schema
on individual databases when using multiple databases in Rails >= 6.1NOTE: Will add integration tests if desired by maintainer
Checklist
master
(if not - rebase it).code introduces user-observable changes.