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

Factorize bin/update in bin/setup, and make bin/setup idempotent #33139

Closed
wants to merge 5 commits into from

Conversation

@davidstosik
Copy link
Contributor

commented Jun 15, 2018

bin/setup and bin/update are currently almost the same file. The only thing that keeps them apart is that one is running bin/rails db:setup and the other bin/rails db:migrate.

I'm suggesting here that they should be a unique script, which also needs to be idempotent.

  • New to a project, need to get started? bin/setup
  • Need to install new dependencies that were added recently? bin/setup

Before deprecating bin/update, I'm suggesting we just have it call bin/setup.

@rails-bot

This comment has been minimized.

Copy link

commented Jun 15, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@@ -27,8 +28,13 @@ chdir APP_ROOT do
# cp 'config/database.yml.sample', 'config/database.yml'
# end

puts "\n== Preparing database =="
system! 'bin/rails db:setup'
if system "bin/rails db:version > /dev/null &>2"

This comment has been minimized.

Copy link
@davidstosik

davidstosik Jun 15, 2018

Author Contributor

Any suggestion of a better way to test whether the database already exists would be welcome! 🙏

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 15, 2018

Member

Maybe there's a rake task / rails command to be extracted here, that handles this? rails db:yes_please_do_the_thing and let it figure out whether to setup or migrate.

This comment has been minimized.

Copy link
@davidstosik

davidstosik Jun 15, 2018

Author Contributor

rails db:migrate_or_setup? 😅

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 15, 2018

Member

Hah, I went for an obviously-ridiculous one because that's the best I could come up with too. 😁

One other thought was db:update, but replacing a bin/update with a bin/setup that calls db:update instead of db:setup might feel a bit too silly.

If we can get some consensus that it's a useful thing to have, though, finding something to call it should be surmountable.

This comment has been minimized.

Copy link
@guilleiguaran

guilleiguaran Jun 15, 2018

Member

wdyt about db:exists returning exit code 1 if the DB doesn't exist?

Right now I'm using it in a setup script:

#!/bin/sh

# Exit on fail
set -e

bundle check || bundle install
yarn check || yarn
rails db:exists && rails db:migrate || rails db:setup

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 17, 2018

Member

I'm not sure how we'd make a db:exists work with multiple databases in the future, and I'm wary that each rails command is pretty slow to run (e.g., we have to boot the full app to read database.yml, because it may contain ERb tags).

I don't particularly object to having such a command, but if the 99% use case is db:exists ? db:migrate : db:setup, then ISTM we should just bundle that into a single unit (which could then consider each database in turn in a multi-DB configuration).

This comment has been minimized.

Copy link
@sj26

sj26 Dec 13, 2018

Contributor

😍

@gmcgibbon gmcgibbon added the railties label Sep 30, 2018

@jeremy jeremy added this to the 6.0.0 milestone Feb 5, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@davidstosik hey!

We are planning to include this in Rails 6.0, can you rebase it? 😊

@davidstosik davidstosik force-pushed the davidstosik:bin-idempotent-setup branch 2 times, most recently from 3ef6fbf to 8ff7421 Feb 6, 2019

@davidstosik

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Just rebased. I'd be happy to provide more work if necessary before this can be merged. 🙂

@@ -26,8 +27,13 @@ FileUtils.chdir APP_ROOT do
# FileUtils.cp 'config/database.yml.sample', 'config/database.yml'
# end

puts "\n== Preparing database =="
system! 'bin/rails db:setup'
if system "bin/rails db:version > /dev/null &>2"

This comment has been minimized.

Copy link
@y-yagi

y-yagi Feb 6, 2019

Member

We should use File::NULL for Windows. Ref: #29958

@davidstosik davidstosik force-pushed the davidstosik:bin-idempotent-setup branch from 489112c to 5b3f242 Feb 6, 2019

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

@davidstosik we have a couple of failures (and potentially outdated tests) in railties/test/application/bin_setup_test.rb, can you check them?

@davidstosik

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Is bin/setup expected to reset the database if it already exists?

In my experience, running bin/setup on a project that has a running database often fails when DROP TABLE IF EXISTS table_name CASCADE fails on a foreign constraint.

Would it be sensible to behave as follows?

  1. If database does not exist, then create it.
  2. If db version is 0, then load database schema and seeds. Any other version number would mean some migrations were run before, and loading db schema and seeds could cause errors or problems.
  3. Run remaining migrations.

How is bin/setup expected to behave when multiple databases exist?

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Would it be sensible to behave as follows?

  1. If database does not exist, then create it.

  2. If db version is 0, then load database schema and seeds. Any other version number would mean some migrations were run before, and loading db schema and seeds could cause errors or problems.

  3. Run remaining migrations.

Yes, in my opinion that would be a good default for bin/setup

How is bin/setup expected to behave when multiple databases exist?

I think using the default is fine for that case:

running bin/rails db:create, bin/rails db:migrate, bin/rails db:drop, and bin/rails db:schema|structure:dump that tasks are run for all relevant envs and all databases in that env

robertomiranda added a commit to robertomiranda/rails that referenced this pull request Mar 27, 2019

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 rails#33139 (comment)
@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Now that #35768 is merged we can move this forward, @davidstosik @robertomiranda please update this to use the new rails db:prepare command and update the tests 🙏

davidstosik added some commits Jun 15, 2018

Factorize bin/update in bin/setup, and make bin/setup idempotent
`bin/setup` and `bin/update` are currently almost the same file. The
only thing that keeps them apart is that one is running `bin/rails
db:setup` and the other `bin/rails db:migrate`.

I'm suggesting here that they should be a unique script, which needs to
be idempotent.

- New to a project, need to get started? `bin/setup`
- Need to install new dependencies that were added recently? `bin/setup`.

Before deprecating `bin/update`, I'm suggesting we just have it call
`bin/setup`.

@davidstosik davidstosik force-pushed the davidstosik:bin-idempotent-setup branch from af824f5 to f53e6e2 Apr 5, 2019

@davidstosik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Rebased, and switched to using the db:prepare idempotent task.

@jeremy

jeremy approved these changes Apr 11, 2019

Copy link
Member

left a comment

Looks good! Let's squash + merge.

@y-yagi

This comment has been minimized.

kaspth added a commit that referenced this pull request Apr 16, 2019

Merge pull request #33139 'bin-idempotent-setup'
Signed-off-by: Kasper Timm Hansen <kaspth@gmail.com>
@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Fixed the tests and pushed this in 8927eba

Pretty damn tough to debug why test_bin_setup_output didn't print creating databases! 😅 Looks like the sqlite3 adapter auto-creates the database because if I put these lines in sqlite3_connection:

diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index f5f5827d04..10c09cbcd4 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -32,10 +32,12 @@ def sqlite3_connection(config)
         Dir.mkdir(dirname) unless File.directory?(dirname)
       end
 
+      puts File.exist?(Rails.root.join("db/development.sqlite3"))
       db = SQLite3::Database.new(
         config[:database].to_s,
         config.merge(results_as_hash: true)
       )
+      puts File.exist?(Rails.root.join("db/development.sqlite3"))
 
       ConnectionAdapters::SQLite3Adapter.new(db, logger, nil, config)
     rescue Errno::ENOENT => error

The development database file isn't there before but suddenly exists afterwards.

@robertomiranda @kamipo do any of you know anything about this? Does this mean that there's a potential state where db:prepare detection mechanism (of relying on NoDatabaseError being thrown) won't work for sqlite3?

@kaspth kaspth closed this Apr 16, 2019

@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Forgot to say thank you for working on this @davidstosik!

@davidstosik davidstosik deleted the davidstosik:bin-idempotent-setup branch Apr 19, 2019

@davidstosik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@kaspth Thank you for merging! Did this require a CHANGELOG addition? (Considering the long term goal would be to retire bin/update...)

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

@kaspth talking with @robertomiranda I mentioned exactly the same, looks like NoDatabaseError isn't raised by SQLite never because database is created automatically if it doesn't exist.

Maybe we should add an special case handling for SQLite in db:prepare ?

@kaspth

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@davidstosik not sure. I think apps will get the new bin/setup when running bin/rails app:update, so perhaps they can glean it from there. I don't know if that command also removes bin/update.

@guilleiguaran yeah, I think that makes a lot of sense! It might make even more sense to remove the reliance on NoDatabaseError from there entirely since it's faulty for 1/3 of the adapters? We might also want to look into other places where NoDatabaseError is used, could be there's other stuff broken for sqlite3.

@davidstosik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@kaspth My choice for now was to keep bin/update and call bin/setup from there, to avoid breaking people's workflow without warning. Should we maybe add some kind of warning explaining that bin/update is deprecated?

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.