Only apply DATABASE_URL for Rails.env by matthewd · Pull Request #14633 · rails/rails

matthewd · 2014-04-07T18:59:20Z

As we like ENV vars, also support DATABASE_URL_#{env}, for more obscure use cases.

This needs more tests:

There's nothing about the 'DATABASE_URL_FOO' handling
And nothing showing that a normally-configured "foo" is unaffected by the DATABASE_URL (unless our current env is called 'foo')

Also, the config method could probably look substantially less awful.
#14616 // @rafaelfranca

Only apply DATABASE_URL for Rails.env

rafaelfranca · 2014-04-07T22:11:43Z

Is this good enough? https://github.com/rafaelfranca/rails/compare/rm-database-url

As we like ENV vars, also support DATABASE_URL_#{env}, for more obscure use cases.

configuration

This seems to simplify the operative part. Most importantly, by pre-loading all the configs supplied in ENV, we ensure the list is complete: if the developer specifies an unknown config, the exception includes a list of valid ones.

If the supplied string doesn't contain a colon, it clearly cannot be a database URL. They must have intended to do a key lookup, so even though it failed, give the explanatory deprecation warning, and raise the exception that lists the known configs. Conveniently, this also simplifies our logical behaviour: if the string matches a known configuration, or doesn't contain a colon (and is therefore clearly not a URL), then we output a deprecation warning, and behave exactly as we would if it were a symbol.

.. even when the supplied config made no hint that name was relevant.

matthewd · 2014-04-08T06:03:04Z

It's feeling like, now that DATABASE_URL doesn't mean override-everything, it should actually get priority over a 'url' key in the hash. I think the previous formulation was basically just a solution to the fact that otherwise, there was no way to avoid it applying to everything.

So, I think the search should go:

DATABASE_URL / DATABASE_URL_FOO
config['url']
config['database'] & friends

My justification is two-pronged: firstly, it seems more reasonable that the runtime environment can override the on-disk config, rather than the other way around; secondly, that sounds like a much simpler fallback process to document clearly. 😁

With my current version, changing that would involve swapping ||= for = on this line.

(This might be a bad idea because of how it interacts with people using <%= ENV['DATABASE_URL'] %> in database.yml.)

rafaelfranca · 2014-04-08T12:16:17Z

I prefer to keep url with priority and my justification is: in the future I want to remove DATABASE_URL.

New generated applications already comes with this configuration:

production:
  url: <%= ENV['DATABASE_URL'] %>

In Rails 5 I want this to be the only way to configure the database. It still provides the flexibility of runtime override but doesn't require a lot of code to merge configurations.

The "DATABASE_URL_*" idea was moving in the wrong direction. Instead, let's deprecate the situation where we end up using ENV['DATABASE_URL'] at all: the Right Way is to explicitly include it in database.yml with ERB.

In passing, allow multi-word adapters to be referenced in a URL: underscored_name must become hyphened-name.

rafaelfranca · 2014-04-08T16:51:59Z

activerecord/test/cases/connection_adapters/connection_handler_test.rb

Now I'm confused. Should not this be using DATABASE_URL?

Duh! I'm really confused, only if :production were the current environment.

Only apply DATABASE_URL for Rails.env

schneems · 2014-04-14T14:16:17Z

activerecord/test/cases/connection_adapters/connection_handler_test.rb

Looks like we're testing to ensure the behavior in this PR does not work https://github.com/rails/rails/pull/14152/files#diff-9e1f52d3449a7a0cfdbd3a7afb5d905bR20

:production is the current env, and there's a valid DATABASE_URL, so we shouldn't get a raise here. Or am I missing something?

The current environment on these tests is called default_env. This is why production is raising.

Great! Thanks for the explanation, that makes sense then.

schneems · 2014-04-14T15:22:25Z

I like that this PR cleaned up quite a bit of code I wasn't proud of. Scoping DATABASE_URL to only the current environment is a very elegant solution that makes quite a bit of sense. Sorry for the delay in the review, thanks for the work all ❤️

prathamesh-sonpatki · 2016-11-13T22:00:59Z

railties/lib/rails/generators/rails/app/templates/config/databases/oracle.yml

+#     url: <%%= ENV['DATABASE_URL'] %>
+#
 production:
-  url: <%%= ENV["DATABASE_URL"] %>


@matthewd @schneems Found this PR which removed ENV["DATABASE_URL"] from production configuration :)

@schneems I was referring to this comment in our discussion - #14633 (comment)

Lol. Glad we caught up in person. We can re-add the DATABASE_URL generation in config/database.yml. I'm still worried about it being taken out (again).

rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this pull request Apr 7, 2014

Merge pull request rails#14633 from matthewd/narrow_database_url

43a2de7

Only apply DATABASE_URL for Rails.env

rafaelfranca added this to the 4.1.0 milestone Apr 7, 2014

matthewd and others added 9 commits April 8, 2014 15:14

Only apply DATABASE_URL for Rails.env

2b817cd

As we like ENV vars, also support DATABASE_URL_#{env}, for more obscure use cases.

Make sure DEFAULT_URL only override current environment database

e533826

configuration

Test DATABASE_URL has precendance over DATABASE_URL_#{environment}

d459f75

Only call DEFAULT_ENV proc one time

2b6d1da

Check env_url only once

d035ba1

entry is always a Hash

0ec0475

Rearrange the config merger some more

de9f2f6

This seems to simplify the operative part. Most importantly, by pre-loading all the configs supplied in ENV, we ensure the list is complete: if the developer specifies an unknown config, the exception includes a list of valid ones.

Ensure we correctly and immediately load all ENV entries

8f23c22

.. even when the supplied config made no hint that name was relevant.

matthewd added 3 commits April 9, 2014 00:17

Less ambition, more deprecation

615e0dc

The "DATABASE_URL_*" idea was moving in the wrong direction. Instead, let's deprecate the situation where we end up using ENV['DATABASE_URL'] at all: the Right Way is to explicitly include it in database.yml with ERB.

Don't deprecate after all

88cd65b

Drop in @jeremy's new database.yml template text

d72a0cb

In passing, allow multi-word adapters to be referenced in a URL: underscored_name must become hyphened-name.

rafaelfranca reviewed Apr 8, 2014
View reviewed changes

rafaelfranca added a commit that referenced this pull request Apr 8, 2014

Merge pull request #14633 from matthewd/narrow_database_url

75e0ee8

Only apply DATABASE_URL for Rails.env

rafaelfranca merged commit 75e0ee8 into rails:master Apr 8, 2014

rafaelfranca mentioned this pull request Apr 8, 2014

With a DATABASE_URL, establish_connection :any_symbol will succeed #14616

Closed

rafaelfranca added a commit that referenced this pull request Apr 8, 2014

Merge pull request #14633 from matthewd/narrow_database_url

ce7e3d5

Only apply DATABASE_URL for Rails.env

rafaelfranca added a commit that referenced this pull request Apr 8, 2014

Merge pull request #14633 from matthewd/narrow_database_url

3a118cc

Only apply DATABASE_URL for Rails.env

schneems reviewed Apr 14, 2014
View reviewed changes

matthewd deleted the narrow_database_url branch August 22, 2015 19:33

prathamesh-sonpatki reviewed Nov 13, 2016

View reviewed changes

Conversation

matthewd commented Apr 7, 2014

Uh oh!

rafaelfranca commented Apr 7, 2014

Uh oh!

matthewd commented Apr 8, 2014

Uh oh!

rafaelfranca commented Apr 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schneems commented Apr 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants