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 db:prepare rake task. #35768

Merged
merged 6 commits into from Apr 2, 2019

Conversation

Projects
None yet
5 participants
@robertomiranda
Copy link
Contributor

commented Mar 27, 2019

In favor of getting merging #33139 (comment). I have implemented rake db:prepare which creates the database, loads the schema, run the migrations and initializes with the seed data
(use db:reset to also drop the database first). This rake task runs in an idempotent way

cc @davidstosik @matthewd @guilleiguaran

Add rake db:prepare rake task.
It Creates the database, loads the schema, run the migrations and initializes with the seed data
(use db:reset to also drop the database first). This rake task runs in an idempotent way

ref #33139 (comment)

@rails-bot rails-bot bot added the activerecord label Mar 27, 2019

@matthewd

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

🙏🏻

This obviously isn't very mutli-DB aware, but it at least exposes an API that we'll be able to maintain when we fix that. 👌🏻 (@eileencodes may have stronger feelings on whether this can merge as is, or should grow that support first.)

I have similar feelings about the implementation tbh.. just connecting then rescuing NoDatabaseError feels a bit rough, but I don't have any immediate better suggestion. (Though I have vague recollection there are cases where we don't actually get that exception -- if we can't connect at all [because the DB we asked for isn't there], we may not be able to tell why.) One thought would be to move the migrate to an else after the rescue, to clarify that the exception is only expected to come from the connect.

We should add a couple of tests, covering the no DB, present-but-behind DB, and already-up-to-date DB cases -- but no need to get exhaustive: we can lean on the existing coverage of the underlying commands for details and edge cases.

@simi

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

🤔 Doesn't this conflict with db:test:prepare? Let's say both prepare database to useful state, but both do that with different approach with different result. Is that ok? Isn't that a little confusing?

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

thanks @matthewd @simi, for your prompt feedback.

This obviously isn't very mutli-DB aware, but it at least exposes an API that we'll be able to maintain when we fix that. 👌🏻 (@eileencodes may have stronger feelings on whether this can merge as is, or should grow that support first.)

Ideally, I was thinking to call something like ActiveRecord::Base.db_exists? but unfortunately at the moment seems like we don't have this API implemented yet. Looking forward to hearing a suggestion from @eileencodes 🙇

Doesn't this conflict with db:test:prepare? Let's say both prepare database to useful state, but both do that with different approach with different result. Is that ok? Isn't that a little confusing?

@simi good catch, I know this is a hard question 😅 but do you have any naming suggestion?

@eileencodes

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I looked at this and making it work for multiple db is pretty simple. We can looop through the configs, try to migrate and if we can't migrate run setup. I tested this on a local app by deleting one of the dbs in a multi-db app and not the others.

Also I recommend the name construct so we don't conflict with prepare.

New code would look like this:

  desc "Runs setup if database does not exist, or runs migrations if it does"
  task construct: :load_config do
    ActiveRecord::Base.configurations.configs_for(env_name: Rails.env).each do |db_config|
      ActiveRecord::Base.establish_connection(db_config.config)

      begin
        db_namespace["migrate"].invoke
      rescue ActiveRecord::NoDatabaseError
        db_namespace["setup"].invoke
      end 
    end 
  end 

I think we can test this in the railties/test//application/rake/multi_dbs_test.rb for multiple databases and potentially also activerecord/test//cases/tasks/database_tasks_test.rb

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

thanks much 🙇 @eileencodes, I gonna implement this suggestion and a couple of test cases on the files that you suggested

@matthewd

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Hmm... I like db:prepare because of the overlap with db:test:prepare: the latter is a specialized version of the general operation, which is "do what you must to get this into a state that I can use it".

I also think it's hard to find an alternative that works for both the create and migrate use cases: e.g. you can definitely construct a database into existence, but I'm not sure about constructing an old database to bring it up to date.

(I also also don't think giving this a different name fixes the potential for confusion with db:test:prepare: whatever it's called, they do nominally do the same thing but have different results. There's a real higher level ambiguity; it's not two very different operations with too-similar names, which can be fixed by moving them further apart.)

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Agree with @matthewd, db:prepare sounds adequate in my opinion.

@rails-bot rails-bot bot added the railties label Apr 2, 2019

@robertomiranda robertomiranda force-pushed the robertomiranda:r/rake-db-prepare branch 3 times, most recently from ede4850 to 6cefe76 Apr 2, 2019

@robertomiranda robertomiranda force-pushed the robertomiranda:r/rake-db-prepare branch from 6cefe76 to 098e4d2 Apr 2, 2019

@robertomiranda robertomiranda force-pushed the robertomiranda:r/rake-db-prepare branch from 2e9d168 to 900e566 Apr 2, 2019

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Hi, 👋 I've test cases for multiple and single DB connection, Also I tested locally with a couple of rails apps and it's looking good. Could you please take a look again?

@eileencodes
Copy link
Member

left a comment

This is looking good but I think we need one more test. Can you add one that drops the db first and then runs prepare and makes sure that create and migrate are both run?

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@eileencodes done 👍

@simi

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

@matthewd @guilleiguaran I'm ok with db:prepare as well, just wanted to ensure db:test:prepare is "taken into account" as well when deciding the name.

@simi

simi approved these changes Apr 2, 2019

@eileencodes eileencodes merged commit 2c4dab1 into rails:master Apr 2, 2019

3 checks passed

buildkite/rails Build #60050 passed (22 minutes, 52 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Thanks @robertomiranda!

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

thanks a lot for all your help 🙇

@robertomiranda robertomiranda deleted the robertomiranda:r/rake-db-prepare branch Apr 3, 2019

@robertomiranda

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

oh actually, I have a question should we update the CHANGELOG?

@simi

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

I think this change deserves changelog change. Feel free to open new PR @robertomiranda.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.