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

Warn if we can't read the yaml to create database tasks #36560

Merged

Conversation

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Jun 26, 2019

Load initial database.yml once, and warn if we can't create tasks

For multiple databases we attempt to generate the tasks by reading the
database.yml before the Rails application is booted. This means that we
need to strip out ERB since it could be reading Rails configs.

In some cases like #36540 the ERB
is too complex and we can't overwrite with the DummyCompilier we used in
#35497. For the complex causes we
simply issue a warning that says we couldn't infer the database tasks
from the database.yml.

While working on this I decided to update the code to only load the
database.yml once initially so that we avoid having to issue the same
warning multiple times. Note that this had no performance impact in my
testing and is merely for not having to save the error off somewhere.
Also this feels cleaner.

Note that this will not break running tasks that exist, it will just
mean that tasks for multi-db like db:create:other_db will not be
generated. If the database.yml is actually unreadable it will blow up
during normal rake task calls.

Fixes #36540

cc/ @tenderlove @jhawthorn @rafaelfranca @klyonrad

@eileencodes eileencodes added this to the 6.0.0 milestone Jun 26, 2019
@eileencodes eileencodes self-assigned this Jun 26, 2019
@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Jun 26, 2019

Oh I'm missing a commit. One sec.

@eileencodes eileencodes force-pushed the warn-if-database-yml-cant-be-read branch from af5ea55 to 64d526a Jun 26, 2019
begin
Rails.application.config.load_database_yaml
rescue
$stdout.puts "Rails couldn't infer whether you are using multiple databases from your database.yml. If you'd like to use this feature, please simplify your ERB."
Copy link
Member Author

@eileencodes eileencodes Jun 26, 2019

Not sure what to say really - maybe we don't warn at all?

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Jun 26, 2019

Ok fixed the commit.

database: db/development.sqlite3

development:
<<: *sqlite
Copy link
Member

@rafaelfranca rafaelfranca Jun 26, 2019

Should not this be an ERB? This seems to be a yaml that we can read without problem.

Copy link
Member Author

@eileencodes eileencodes Jun 26, 2019

Oops yes, will fix that.

Copy link
Member Author

@eileencodes eileencodes Jun 26, 2019

Actually this is straight up a bad test 😢 Working on fixing it.

@eileencodes eileencodes force-pushed the warn-if-database-yml-cant-be-read branch from 64d526a to 4eb7b40 Jun 26, 2019
begin
Rails.application.config.load_database_yaml
rescue
$stdout.puts "Rails couldn't infer whether you are using multiple databases from your database.yml. If you'd like to use this feature, please simplify your ERB."
Copy link
Member

@rafaelfranca rafaelfranca Jun 26, 2019

$stderr maybe?

I think for the message we could do:

"Rails couldn't infer whether you are using multiple databases from your database.yml and can't generate the test for the non-primary databases. If you'd like to use this feature, please simplify your ERB."

Copy link
Member Author

@eileencodes eileencodes Jun 27, 2019

Done.

@@ -2,6 +2,8 @@

require "active_record"

@databases = ActiveRecord::Tasks::DatabaseTasks.setup_initial_database_yaml
Copy link
Member

@rafaelfranca rafaelfranca Jun 26, 2019

I think a local variable works, but I'm fine with an instance variable

Copy link
Member Author

@eileencodes eileencodes Jun 27, 2019

Fixed.

For multiple databases we attempt to generate the tasks by reading the
database.yml before the Rails application is booted. This means that we
need to strip out ERB since it could be reading Rails configs.

In some cases like rails#36540 the ERB
is too complex and we can't overwrite with the DummyCompilier we used in
rails#35497. For the complex causes we
simply issue a warning that says we couldn't infer the database tasks
from the database.yml.

While working on this I decided to update the code to only load the
database.yml once initially so that we avoid having to issue the same
warning multiple times. Note that this had no performance impact in my
testing and is merely for not having to save the error off somewhere.
Also this feels cleaner.

Note that this will not break running tasks that exist, it will just
mean that tasks for multi-db like `db:create:other_db` will not be
generated. If the database.yml is actually unreadable it will blow up
during normal rake task calls.

Fixes rails#36540
@eileencodes eileencodes force-pushed the warn-if-database-yml-cant-be-read branch from 4eb7b40 to df6b0de Jun 27, 2019
@eileencodes eileencodes merged commit 4024612 into rails:master Jun 27, 2019
2 checks passed
@eileencodes eileencodes deleted the warn-if-database-yml-cant-be-read branch Jun 27, 2019
eileencodes added a commit that referenced this issue Jun 27, 2019
…be-read

Warn if we can't read the yaml to create database tasks
@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Jun 27, 2019

Backported to 6-0-stable

pull bot pushed a commit to Mattlk13/rails that referenced this issue Jul 25, 2019
In rails#36560 I accidentally re-introduced a bug where ActiveRecord can't be
used without Rails. This returns an empty hash if we're outside the
context of Rails since we can't create the database tasks without
loading and reading the database yaml which is something only Railties
can do.
eileencodes added a commit that referenced this issue Jul 25, 2019
In #36560 I accidentally re-introduced a bug where ActiveRecord can't be
used without Rails. This returns an empty hash if we're outside the
context of Rails since we can't create the database tasks without
loading and reading the database yaml which is something only Railties
can do.
@jasonperrone
Copy link

@jasonperrone jasonperrone commented Nov 27, 2019

Is there a way to disable this message if you're not interested in multiple database functionality and you don't want it to try inspecting your database.yml file?

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 27, 2019

The only thing would be to change your YAML to something that Rails can read. The problem is with some more complex YAML we can't tell whether you want multi-db or not.

@jasonperrone
Copy link

@jasonperrone jasonperrone commented Nov 27, 2019

So our database.yml is made complex with ERB because we're using it to support multiple databases already, but not in the way that this new feature is intended. For example, our app is deployed once, but depending on the URL connects to a different database. Just multi-tenancy. It appears to me that this new feature is more geared towards one instance using two databases, one for read only actions and then one for writing. Nice feature, but it's not quite what we're doing. The yaml in our files helps us quickly define entries based on "environment type" and other factors that allow us to enumerate over our 60 databases and configure them in groups. I'm probably not explaining right so the bottom line is I can't take the ERB out of this file and I'd just love the message to go away.

@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Nov 27, 2019

At the moment I don't have time to work on hiding this message but we'll accept a PR that does.

suketa added a commit to suketa/rails_sandbox that referenced this issue Dec 14, 2019
Warn if we can't read the yaml to create database tasks
rails/rails#36560
suketa added a commit to suketa/rails_sandbox that referenced this issue Dec 14, 2019
Warn if we can't read the yaml to create database tasks
rails/rails#36560
@OmriSama
Copy link

@OmriSama OmriSama commented Oct 29, 2020

@jasonperrone I added this feature in #40449 :) enjoy!

@jasonperrone
Copy link

@jasonperrone jasonperrone commented Oct 30, 2020

Awesome, thank you :)

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.

4 participants