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

Fixed rails dbconsole to support DATABASE_URL #13328

Merged
merged 1 commit into from
Dec 16, 2013

Conversation

teohm
Copy link
Contributor

@teohm teohm commented Dec 15, 2013

Fixes #13320.

Problem
rails dbconsole raises error when ENV['DATABASE_URL'] exists and config/database.yml removed (see /issues/13320).

Solution
ActiveRecord and rake db:* uses ConnectionSpecification::Resolver to loads DATABASE_URL properly. This patch modifies rails dbconsole to use the same class.

@robin850
Copy link
Member

Awesome @teohm! Could you add a changelog entry please ?

ActiveRecord::ConnectionAdapters::ConnectionSpecification::Resolver.new(
ENV['DATABASE_URL'],
Rails.application.config.database_configuration.to_h
).spec.config.to_h.stringify_keys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to_h is only available with Ruby 2.0+ so this is the reason why the build fail with 1.9.3 on Travis.

@teohm
Copy link
Contributor Author

teohm commented Dec 15, 2013

@robin850 One more minor thing I wish to discuss.

Currently I let all errors raised in ConnectionSpecification::Resolver (AdapterNotSpecified, Gem::LoadError, LoadError) to pass through. So both error message and backtrace will be printed on terminal, beceause I feel the backtrace can be useful for developers.

But I'm looking for a 2nd opinion, what do you think?

@senny
Copy link
Member

senny commented Dec 16, 2013

@teohm passing exceptions through is 👍


app_db_file("test:\n without_init")
assert_equal Rails::DBConsole.new.config, "without_init"
Rails::DBConsole.const_set('APP_PATH', 'rails/all')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will leak outside of this test case. Can you define it in setup and remove it in teardown?

@senny
Copy link
Member

senny commented Dec 16, 2013

@teohm added a few minor comments. Let me know when you updated.

@teohm
Copy link
Contributor Author

teohm commented Dec 16, 2013

@senny, thanks for your code review! I have updated the commits:

  1. removed app_config, app_db_file.
  2. properly setup/teardown the fake APP_PATH
  3. moved test helper method app_db_config() to private section.

@senny
Copy link
Member

senny commented Dec 16, 2013

@teohm 👍 can you squash your commits?

@teohm
Copy link
Contributor Author

teohm commented Dec 16, 2013

@senny squashed.

senny added a commit that referenced this pull request Dec 16, 2013
Fixed rails dbconsole to support DATABASE_URL
@senny senny merged commit bd66532 into rails:master Dec 16, 2013
@senny
Copy link
Member

senny commented Dec 16, 2013

@teohm thank you for your contribution 💛

@teohm
Copy link
Contributor Author

teohm commented Dec 16, 2013

@senny thanks for the merge! 🍻

senny added a commit that referenced this pull request Dec 16, 2013
Fixed rails dbconsole to support DATABASE_URL
Conflicts:
	railties/CHANGELOG.md
@schneems
Copy link
Member

This looks great ❤️ thanks for the work here 😄

@robin850
Copy link
Member

Awesome guys! 🍰 (sorry for the late reply though @teohm)

@teohm
Copy link
Contributor Author

teohm commented Dec 16, 2013

@schneems @robin850 thanks for the review too 😸 :octocat:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rails dbconsole raise error when ENV['DATABASE_URL'] exists and config/database.yml removed.
5 participants