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 rails db:system:change command #34832

Merged
merged 4 commits into from Jan 16, 2019

Conversation

Projects
None yet
7 participants
@gmcgibbon
Copy link
Member

gmcgibbon commented Dec 31, 2018

Part two of #34710.

After studying up commands and generators, I decided to make a command that delegates to a generator (similar to rails new / app generator). This seems to give us the most flexibility with how the command interacts with files, and if the user wants to accept the config overwrites or not.

TODO:

  • Fix three level nested generator lookup
  • Add tests for generator with file assertions
  • Add chagelog entry

@rails-bot rails-bot bot added the railties label Dec 31, 2018

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_system_change_command branch 8 times, most recently from 9342aa9 to 6e19f90 Dec 31, 2018

@eileencodes
Copy link
Member

eileencodes left a comment

I like this. I left 2 minor comments.

when "jdbcsqlite3" then ["activerecord-jdbcsqlite3-adapter", nil]
when "jdbcpostgresql" then ["activerecord-jdbcpostgresql-adapter", nil]
when "jdbc" then ["activerecord-jdbc-adapter", nil]
else [database, nil]

This comment has been minimized.

@eileencodes

eileencodes Jan 2, 2019

Member

Should this raise an error if we get to the else? Otherwise I think this will silently fail.

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 2, 2019

Author Member

I don't think so, sqlite3 and ibm_db have a direct adapter name to gem name mapping with no version locking. The value itself should be validated by the #initialize check in ChangeGenerator.

Show resolved Hide resolved railties/lib/rails/generators/rails/db/system/change/change_generator.rb Outdated

if context
lookups << "#{name}:#{context}"
end

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

What do these new lines do? I've always found the base and context naming here confusing?

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 3, 2019

Author Member

I'm essentially only adding lookups << "rails:#{base}:#{name}" if a base exists. This is necessary due to the second part of the lookup code:

unless base || context
  unless name.to_s.include?(?:)
    lookups << "#{name}:#{name}"
    lookups << "rails:#{name}"
  end
  lookups << "#{name}"
end

Specifically, lookups << "rails:#{name}" which limits us to looking up single segment generator names in the toplevel rails namespace.

When looking up rails g db:system:change, name is the last segment ("change"), base is the namespace path ("db:system"), and context is nil. I think context is used if name isn't the command name, but I'm not sure why. Maybe we should change base to path?

This comment has been minimized.

@kaspth

kaspth Jan 3, 2019

Member

So it's possible for users to call rails g db:system:change? I thought this generator was internal and sufficiently called via it's class start method from the command?

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 3, 2019

Author Member

That is currently possible, yes. If we don't want it to be possible, and only called via the command, I can remove this patch to #find_by_namespace. WDYT?

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 9, 2019

Author Member

I'll go ahead and revert the changes to the generator lookup and hide this generator so it can only be called through the command. I'm assuming we would prefer only one way of changing databases.

@kaspth
Copy link
Member

kaspth left a comment

You're getting pretty adept at finding your way around the codebase! Lovely to see 💪

module Db
module System
class ChangeCommand < Base # :nodoc:
class_option :to, desc: "The DBMS to switch to."

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

Do we generally say DBMS elsewhere?

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 3, 2019

Author Member

Only once in a comment of activerecord/lib/active_record/connection_adapters/abstract_adapter.rb. I see RDBMS in guides/source/active_record_basics.md and activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb, but I don't think that's much better. I'll change it to database for now.

This comment has been minimized.

@matthewd

matthewd Jan 3, 2019

Member

Try "database type"? It's not brilliant, but maybe an improvement? Or "database platform"?

This comment has been minimized.

@kaspth

kaspth Jan 3, 2019

Member

Using "database system" would match the command name and reinforce the concept. But it's perhaps not expounding on what a "database system" means, that meaning only comes from seeing MySQL, Postgres and the gang.

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 3, 2019

Author Member

Agree that database system makes the most sense given the command name. Will use that

Show resolved Hide resolved railties/lib/rails/generators/app_name.rb Outdated
Show resolved Hide resolved railties/lib/rails/generators/app_name.rb Outdated

def valid_const?
if /^\d/.match?(app_const)
raise Error, "Invalid application name #{original_app_name}. Please give a name which does not start with numbers."

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

Curiosity for another time: since 2.6 allows constants to start with non ascii characters, does that mean we could name a Rails app, say, 666?

This comment has been minimized.

@jrochkind

jrochkind Jan 23, 2019

Contributor

@kaspth not 666, because it needs to begin with something the unicode database as included in the particular ruby version thinks is an uppercase letter. Just not limited to ascii.

Show resolved Hide resolved railties/lib/rails/generators/database.rb Outdated
Show resolved Hide resolved railties/lib/rails/generators/rails/db/system/change/change_generator.rb Outdated
Show resolved Hide resolved railties/lib/rails/generators/rails/db/system/change/change_generator.rb Outdated
gem 'turbolinks', '~> 5'
# Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
gem 'jbuilder', '~> 2.5'
GEMFILE

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

How will we have to maintain these Gemfiles going forward?

This comment has been minimized.

@gmcgibbon

gmcgibbon Jan 3, 2019

Author Member

Good question. I initially tried to generate all test apps without --skip-gemfile, but then rails will run bundle install and crash because rails 6 isn't released yet. I just noticed we have skip_bundle as an app generator option, so I'll try using that, and using the gem template in generator tests.

assert_match "adapter: postgresql", content
assert_match "database: test_app", content
end
assert_file("Gemfile") do |content|

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

Adding a newline after output and between the assert_file seems like it would increase readability in these tests.

module Command
module Db
module System
class ChangeCommand < Base # :nodoc:

This comment has been minimized.

@kaspth

kaspth Jan 2, 2019

Member

For another time: seeing this deep module nesting makes me think we should consider adding something akin to Action Mailboxes' routing for commands.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jan 2, 2019

Forgot to ask, how are we handling cases where people who may have changed their config/database.yml when calling this command?

@gmcgibbon gmcgibbon changed the title [WIP] Add rails db:system:change command Add rails db:system:change command Jan 2, 2019

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_system_change_command branch from b0a95b7 to 9c2ef0f Jan 3, 2019

@gmcgibbon

This comment has been minimized.

Copy link
Member Author

gmcgibbon commented Jan 3, 2019

Forgot to ask, how are we handling cases where people who may have changed their config/database.yml when calling this command?

The generator will detect a configuration file already exists and gives options to overwrite, show a diff, reject the change, or merge the two files together.

Eg.

rails g db:system:change --to mysql
    conflict  config/database.yml
Overwrite /Users/gannon/dummy/config/database.yml? (enter "h" for help) [Ynaqdhm] h
        Y - yes, overwrite
        n - no, do not overwrite
        a - all, overwrite this and all others
        q - quit, abort
        d - diff, show the differences between the old and the new
        h - help, show this help
        m - merge, run merge tool
Overwrite /Users/gannon/dummy/config/database.yml? (enter "h" for help) [Ynaqdhm]

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_system_change_command branch from 9c2ef0f to 55a27f4 Jan 3, 2019

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_system_change_command branch 2 times, most recently from 2131e71 to 88e1d38 Jan 9, 2019

gmcgibbon added some commits Dec 31, 2018

Add rails db:system:change command
Add `rails db:system:change` command for changing databases.

```
bin/rails db:system:change --to=postgresql
   force  config/database.yml
    gsub  Gemfile
```

The change command copies a template `config/database.yml` with
the target database adapter into your app, and replaces your database
gem with the target database gem.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:db_system_change_command branch from 88e1d38 to d49899c Jan 9, 2019

@gmcgibbon

This comment has been minimized.

Copy link
Member Author

gmcgibbon commented Jan 11, 2019

I've hidden the change generator, reverted the patch to call it with rails g, and removed the static gemfile strings in favour of template evaluation. Let me know if there's anything else this PR needs to get shipped!

@kaspth kaspth merged commit 90536eb into rails:master Jan 16, 2019

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bogdanvlviv

This comment has been minimized.

Copy link
Contributor

bogdanvlviv commented Jan 21, 2019

Hey, Thanks for adding this!
Since this PR is merged we should close #34710, and #34732?
Also, I think we should exclude db:change from commands (railties/lib/rails/command.rb file) in order to not show it on rails --help, (we should show db:system:change instead).
Sorry for the too late review.

Edited: Seems like Rails::Command::Db::System::ChangeCommand.printing_commands returns ["db:change"] instead of [db:system:change].

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jan 23, 2019

@gmcgibbon

This comment has been minimized.

Copy link
Member Author

gmcgibbon commented Jan 23, 2019

Thanks for pointing that out @bogdanvlviv, I'll see what I can do!

@gmcgibbon gmcgibbon deleted the gmcgibbon:db_system_change_command branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment