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

Allow to pass a connection to the `dbconsole` command #29358

Merged
merged 3 commits into from Jul 16, 2017

Conversation

@robin850
Copy link
Member

robin850 commented Jun 5, 2017

Hello,

Since 0a4f600, it's possible to specify a 3-level database configuration to gather connections by environment.

The dbconsole command will try to look for a database configuration which points to the current environment but with such flavour, the environment key is flushed out so let's add the ability to specify the connection and pick primary by default to be consistent with Active Record.

Have a nice day !

@robin850 robin850 added the railties label Jun 5, 2017
railties/lib/rails/commands/dbconsole/dbconsole_command.rb Outdated
@@ -99,6 +99,10 @@ def environment
Rails.respond_to?(:env) ? Rails.env : Rails::Command.environment
end

def connection
@options[:connection] || "primary"

This comment has been minimized.

@y-yagi

y-yagi Jun 5, 2017 Member

Can we use thor's default value?

      class_option :connection, aliases: "-c", type: :string,
        desc: "Specifies the connection to use.", default: "primary"

This comment has been minimized.

@robin850

robin850 Jun 5, 2017 Author Member

Yes, totally fair point and thus, this will be printed running --help, thanks !

This comment has been minimized.

@robin850

robin850 Jun 6, 2017 Author Member

Actually I added it back because Rails::DBConsole is public API so people may be using it directly, without Thor. 😊

@robin850 robin850 force-pushed the robin850:dbconsole-connection branch 2 times, most recently Jun 5, 2017
Copy link
Member

jeremy left a comment

Yes please!

railties/lib/rails/commands/dbconsole/dbconsole_command.rb Outdated
@@ -87,10 +87,10 @@ def start

def config
@config ||= begin
if configurations[environment].blank?
if configurations[environment].blank? && configurations[connection].blank?
raise ActiveRecord::AdapterNotSpecified, "'#{environment}' database is not configured. Available configuration: #{configurations.inspect}"

This comment has been minimized.

@jeremy

jeremy Jun 8, 2017 Member

This error will be misleading if the connection was provided.

railties/lib/rails/commands/dbconsole/dbconsole_command.rb Outdated
raise ActiveRecord::AdapterNotSpecified, "'#{environment}' database is not configured. Available configuration: #{configurations.inspect}"
else
configurations[environment]
configurations[environment] || configurations[connection]

This comment has been minimized.

@jeremy

jeremy Jun 8, 2017 Member

If configurations[environment] is {} and configurations[connection] is present, we end up returning the blank env config here instead of the conn config.

railties/lib/rails/commands/dbconsole/dbconsole_command.rb Outdated
def connection
@options["connection"] || "primary"
end

This comment has been minimized.

@jeremy

jeremy Jun 8, 2017 Member

Nice case for Hash#fetch

railties/lib/rails/commands/dbconsole/dbconsole_command.rb Outdated
@@ -145,6 +149,9 @@ class DbconsoleCommand < Base # :nodoc:
class_option :environment, aliases: "-e", type: :string,
desc: "Specifies the environment to run this console under (test/development/production)."

class_option :connection, aliases: "-c", type: :string,
desc: "Specifies the connection to use.", default: "primary"

def perform
extract_environment_option_from_argument

This comment has been minimized.

@jeremy

jeremy Jun 8, 2017 Member

I'd expect to be able to provide the conn as a plain arg. Example scenarios/possibilities:

rails dbconsole production replica

rails dbconsole production:replica

RAILS_ENV=production rails dbconsole replica

rails dbconsole reporting-replica

This comment has been minimized.

@kaspth

kaspth Jun 8, 2017 Member

Think we should deprecate the positional environment arg then and have:

rails dbconsole replica -e production
railties/CHANGELOG.md Outdated
command when using a 3-level database configuration.

*Robin Dupret*

This comment has been minimized.

@jeremy

jeremy Jun 8, 2017 Member

Let's show an example of this invocation style, too 👍

@robin850 robin850 force-pushed the robin850:dbconsole-connection branch Jun 8, 2017
@robin850
Copy link
Member Author

robin850 commented Jun 8, 2017

Jeremy, there is now a presence check in case configurations[environment] is {} but I don't see how that could happen because this would've been catch by blank? above. Also, with the new configuration system, Active Record is either returning a) a configuration with the environment's as a key (2-level db config) or b) a configuration without any occurrence of the current environment but rather a key for each possible connection (3-level db config). We can keep it but I'm not sure this is useful.

Kasper, passing the environment as a regular argument is deprecated as you suggested ; this is certainly going to be easier for us to handle only the connection as a regular argument. Such flavor is not deprecated for the console command as I don't know whether we are going to open all available connections by default (c.f. #29360) or let people open them manually (thus, we would have to do that for console as well).

Finally, there's a tiny fix attached with this pull request because if we are going to deprecate passing the environment as a regular argument, people may encounter issues as the -e notation didn't expand shortcuts (like dev for development).

Thanks for the feedback guys ! :-)

@kaspth
Copy link
Member

kaspth commented Jul 15, 2017

Can you rebase? The changelog conflicts.

I think we should deprecate the environment positional command argument for console as well for consistency.

robin850 added 3 commits Jun 5, 2017
Since 0a4f600, it's possible to specify a 3-level database
configuration to gather connections by environment.

The `dbconsole` command will try to look for a database configuration
which points to the current environment but with such flavour, the
environment key is flushed out so let's add the ability to specify
the connection and pick `primary` by default to be consistent with
Active Record.
People should rather rely on the `-e` or `--environment` options to
specify in which environment they want to work. This will allow us
to specify the connection to pick as a regular argument in the future.
Running the `console` and `dbconsole` commands with a regular argument
as the environment's name automatically expand it to match an existing
environment (e.g. dev for development).

This feature wasn't available using the `--environment` (a.k.a `-e`)
option.
@robin850 robin850 force-pushed the robin850:dbconsole-connection branch to 3777701 Jul 16, 2017
@robin850
Copy link
Member Author

robin850 commented Jul 16, 2017

@kaspth : Rebased and deprecated the environment argument for the console command.

end

private
def extract_environment_option_from_argument
if environment
self.options = options.merge(environment: acceptable_environment(environment))
elsif !options[:environment]

ActiveSupport::Deprecation.warn "Passing the environment's name as a " \

This comment has been minimized.

@kaspth

kaspth Jul 16, 2017 Member

Much cleaner to have this straight in here 👏

@kaspth kaspth merged commit c74820d into rails:master Jul 16, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@robin850 robin850 deleted the robin850:dbconsole-connection branch Jul 16, 2017
gsamokovarov added a commit to gsamokovarov/rails that referenced this pull request Mar 4, 2018
I mistype `rails server production` instead of `rails server -e
production` expecting to lunch a server in the production environment
all the time. However, the signature of `rails server --help` is:

```
Usage:
  rails server [puma, thin etc] [options]
```

This means that the `production` argument is being interpreted as a Rack
server handler like Puma, Thin or Unicorn.

Should we argue for the `rails server production`? I'm not sure of the
reasons, but the `rails console production` behavior was deprecated in:
rails#29358, so parity with the existing
`rails console production` usage may not hold anymore.

In any case, this PR introduces an explicit option for the Rack servers
configuration. The option is called `--using` (or `-u` for short) to
avoid the `rails server --server` tantrum.

The new interface of `rails server` is:

```
Usage:
  rails server [using] [options]

Options:
  -p, [--port=port]                        # Runs Rails on the specified port - defaults to 3000.
  -b, [--binding=IP]                       # Binds Rails to the specified IP - defaults to 'localhost' in development and '0.0.0.0' in other environments'.
  -c, [--config=file]                      # Uses a custom rackup configuration.
                                           # Default: config.ru
  -d, [--daemon], [--no-daemon]            # Runs server as a Daemon.
  -e, [--environment=name]                 # Specifies the environment to run this server under (development/test/production).
  -u, [--using=name]                       # Specifies the Rack server used to run the application (thin/puma/webrick).
  -P, [--pid=PID]                          # Specifies the PID file.
                                           # Default: tmp/pids/server.pid
  -C, [--dev-caching], [--no-dev-caching]  # Specifies whether to perform caching in development.
      [--early-hints], [--no-early-hints]  # Enables HTTP/2 early hints.
```

As a bonus, if you mistype the server to use, you'll get an
auto-correction message:

```
$ rails s tin
Could not find handler "tin". Maybe you meant "thin" or "cgi"?
Run `rails server --help` for more options.
```
abicky added a commit to abicky/inf-ruby that referenced this pull request Jan 29, 2019
This commit resolves deprecation warnings like below:

>DEPRECATION WARNING: Passing the environment's name as
a regular argument is deprecated and will be removed
in the next Rails version. Please, use the -e option instead.

cf. rails/rails#29358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.