-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Only apply DATABASE_URL for Rails.env #14633
Conversation
Only apply DATABASE_URL for Rails.env
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.
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.
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:
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 (This might be a bad idea because of how it interacts with people using |
I prefer to keep 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.
actual = resolve(:production, config) | ||
expected = { "adapter"=>"postgresql", "database"=>"foo", "host"=>"localhost" } | ||
expected = { "adapter"=>"not_postgres", "database"=>"not_foo", "host"=>"localhost" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm confused. Should not this be using DATABASE_URL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duh! I'm really confused, only if :production
were the current environment.
Only apply DATABASE_URL for Rails.env
Only apply DATABASE_URL for Rails.env
Only apply DATABASE_URL for Rails.env
ENV['DATABASE_URL'] = "postgres://localhost/foo" | ||
config = { "not_production" => { "adapter" => "not_postgres", "database" => "not_foo" } } | ||
assert_raises AdapterNotSpecified do | ||
spec("production", config) | ||
resolve(:production, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current environment on these tests is called default_env
. This is why production
is raising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for the explanation, that makes sense then.
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 ❤️ |
production: | ||
url: <%%= ENV["DATABASE_URL"] %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schneems I was referring to this comment in our discussion - #14633 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
As we like ENV vars, also support DATABASE_URL_#{env}, for more obscure use cases.
This needs more tests:
Also, the
config
method could probably look substantially less awful.#14616 // @rafaelfranca