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

Workaround Rails.configuration.database_configuration being {} #15269

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jun 1, 2017

If you don't have a config/database.yml and only ENV["DATABASE_URL"], Rails.configuration.database_configuration will be {}, so you'll get an undefined [] for nil error.

While attempting to fix that in rails/rails#29305, I was told that it was not intended to be the public interface. So I'm changing this PR to always use ActiveRecord::Base.configurations instead.

@bdunne bdunne requested review from jrafanie, Fryguy and NickLaMuro and removed request for NickLaMuro June 1, 2017 03:02
@NickLaMuro
Copy link
Member

I see I was booted of the reviewer list here, but I think this looks fine from my end (as far as it shouldn't break what I had already). Not sure we want to silently pass on the host not being there though, but maybe I am missing something of concern here.

@matthewd
Copy link
Contributor

matthewd commented Jun 1, 2017

Per rails/rails#29305 (comment), this should always be using ActiveRecord::Base.configurations. (Unless it's sometimes run before the configuration is loaded, or something? In which case I'd probably try to rearrange the order instead of recreating the config-reading process, tbh.)

If you don't have a config/database.yml and only ENV["DATABASE_URL"],
Rails.configuration.database_configuration will be {}, so you'll get an
undefined [] for nil error.

While attempting to fix that in rails/rails#29305,
I was told that it was not intended to be the public interface.  So I'm
changing this PR to always use ActiveRecord::Base.configurations instead.
@bdunne
Copy link
Member Author

bdunne commented Jun 1, 2017

Thanks @matthewd I updated this PR accordingly

@bdunne
Copy link
Member Author

bdunne commented Jun 1, 2017

@NickLaMuro I thought you were away 🏖️ , so I wanted you to see it, but not require your response.

@miq-bot
Copy link
Member

miq-bot commented Jun 1, 2017

Checked commit bdunne@3a6cf41 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@NickLaMuro
Copy link
Member

@bdunne Stay-cation... but yeah.

Regarding the defined? Rails portion, I only left that part in to keep the old compatibility in place in case that was preferred/had an edge case in place (but thanks to @matthewd 's comment, it seems like it isn't).

@jrafanie
Copy link
Member

jrafanie commented Jun 1, 2017

Oh wow, TIL... I would never have guessed that the seemingly lower level ActiveRecord::Base.configurations is actually the public interface but the seemingly higher level Rails.configuration.database_configuration is really more "private".

It makes sense to do it this way if AR::Base.configurations is the public way of doing it.

@jrafanie jrafanie self-assigned this Jun 1, 2017
@jrafanie jrafanie merged commit 86e8752 into ManageIQ:master Jun 1, 2017
@jrafanie jrafanie added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 1, 2017
@bdunne bdunne deleted the workaround_rails_29305 branch June 1, 2017 18:36
jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 26, 2020
Rails.configuration.database_configuration has no knowledge of db configuration
found in the DATABASE_URL environment variable.  We need to use
ActiveRecord::Base.configurations.

Related to: ManageIQ#15269
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request May 26, 2020
…figuration

found in the DATABASE_URL environment variable. We need to use
ActiveRecord::Base.configurations.

Related to: ManageIQ/manageiq#20208
And: ManageIQ/manageiq#15269
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request May 26, 2020
Rails.configuration.database_configuration has no knowledge of db configuration
found in the DATABASE_URL environment variable. We need to use
ActiveRecord::Base.configurations.

Related to: ManageIQ/manageiq#20208
And: ManageIQ/manageiq#15269
jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 26, 2020
Rails.configuration.database_configuration has no knowledge of db configuration
found in the DATABASE_URL environment variable.  We need to use
ActiveRecord::Base.configurations.

Related to: ManageIQ#15269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants