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

Update retrieving database config hash for Rails >= 6.1 #70

Closed
wants to merge 4 commits into from

Conversation

JuanitoFatas
Copy link
Contributor

Somehow I missed this one in #67. Changed the way to retrieve the database config hash because the old way will stop working in Rails 6.2.

- database_configuration["connection_name"]
+ database_configuration.configs_for(env_name: "test").configuration_hash

This PR fixes following warnings:

DEPRECATION WARNING: [] is deprecated and will be removed from Rails 6.2
(Use configs_for) (called from create_cleaner at
lib/database_rewinder.rb:19)

DEPRECATION WARNING: DatabaseConfig#config will be removed in 6.2.0 in
favor of DatabaseConfigurations#configuration_hash which returns a hash
with symbol keys (called from get_cache_key at
lib/database_rewinder.rb:87)

@eileencodes Does this look good to you? Thanks!

Rails 6.1 changed the way to retrieve the database config hash to

DatabaseConfigurations#configs_for(env_name: "test").configuration_hash

The old way will stop working in Rails 6.2.

DEPRECATION WARNING: [] is deprecated and will be removed from Rails 6.2
(Use configs_for) (called from create_cleaner at
lib/database_rewinder.rb:19)

DEPRECATION WARNING: DatabaseConfig#config will be removed in 6.2.0 in
favor of DatabaseConfigurations#configuration_hash which returns a hash
with symbol keys (called from get_cache_key at
lib/database_rewinder.rb:87)
@@ -82,16 +82,28 @@ def all_table_names(connection)
end
end

def get_config_from(connection_name)
if Gem::Version.new(Rails.version) >= Gem::Version.new("6.1.0.alpha")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileencodes Should we check configs_for change for Rails 6.0 or 6.1? I found the refactoring introduced in rails/rails#32274 which says v6.0.0.beta1.

Choose a reason for hiding this comment

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

configs_for has been available since 6.0. The hash implementation will be removed in 6.2.

lib/database_rewinder.rb Outdated Show resolved Hide resolved
takkanm added a commit to takkanm/database_rewinder that referenced this pull request Oct 17, 2020
`ActiveRecord::Base.configurations` in edge rails returns Symbol key.
And I watched the following discussion.
amatsuda#70 (comment)
I was referring to this comment as well.
@crigor crigor mentioned this pull request Dec 17, 2020
@JuanitoFatas JuanitoFatas marked this pull request as ready for review December 17, 2020 07:04
Comment on lines +86 to +87
if Gem::Version.new(Rails.version) >= Gem::Version.new("6.0.0")
database_configuration.configs_for(env_name: connection_name).first.configuration_hash
Copy link

Choose a reason for hiding this comment

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

configs_for is available on Rails 6.0 but configuration_hash is only available on Rails 6.1. This causes the error on the build.

def get_cache_key(connection_pool)
if connection_pool.respond_to?(:db_config) # ActiveRecord >= 6.1
connection_pool.db_config.config
if Gem::Version.new(Rails.version) >= Gem::Version.new("6.0.0")
Copy link

Choose a reason for hiding this comment

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

Do we need this check? The line above already checks for ActiveRecord >= 6.1. In any case, configuration_hash is only available on Rails 6.1.

@mtgto
Copy link

mtgto commented Mar 16, 2021

@JuanitoFatas How's it going this pull request?
i also awaiting to use this gem in rails 6.1 😺

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.

5 participants