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

Read Database Config From Env Variable For Rails/BulkChangeTable #903

Merged

Conversation

joergschiller
Copy link
Contributor

@joergschiller joergschiller commented Jan 2, 2023

Rails/BulkChangeTable currently reads information about the adapter from the development environment in config/database.yml. It's not unusual to use the environment variable DATABASE_URL instead of having a configuration file on CI (see 1 or 2).

In this case the cop can't figure out if the adapter is supported and hence not warn about the potential use of change_table. Something that happens to me today.

This PR tries to get the adapter from DATABASE_URL after trying to read from config/database.yml.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@joergschiller joergschiller changed the title Read database config from environment variable Read database config from environment variable for Rails/BulkChangeTable Jan 2, 2023
@joergschiller joergschiller changed the title Read database config from environment variable for Rails/BulkChangeTable Database Config From Env Variable For Rails/BulkChangeTable Jan 2, 2023
@joergschiller joergschiller changed the title Database Config From Env Variable For Rails/BulkChangeTable Read Database Config From Env Variable For Rails/BulkChangeTable Jan 2, 2023
case url
when %r{\Amysql2://}
MYSQL
when %r{\Apostgres(ql)?://}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joergschiller joergschiller marked this pull request as ready for review January 2, 2023 23:27
@joergschiller
Copy link
Contributor Author

Something I found while trying to debug the difference in RuboCop offenses between our CI and the local development environment:

https://gitlab.com/gitlab-org/customers-gitlab-com/-/issues/1029#note_499041793

@koic
Copy link
Member

koic commented Sep 6, 2023

@joergschiller Looks good to me. Can you squash your commits into one?

@@ -207,6 +207,18 @@ def database_yaml
nil
end

def database_from_env
url = ENV['DATABASE_URL'].presence
Copy link
Member

@koic koic Sep 6, 2023

Choose a reason for hiding this comment

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

It would be good to document this ENV['DATABASE_URL'] behavior.

@joergschiller joergschiller force-pushed the database_from_env_bulk_change_table branch from 6845469 to bda5de3 Compare September 6, 2023 09:19
@joergschiller
Copy link
Contributor Author

@koic thanks for considering the changes. I've added a short description about the behaviour to the cop documentation. I also rebased on master and squashed the commits.

@koic koic merged commit 38e408a into rubocop:master Sep 6, 2023
10 checks passed
@koic
Copy link
Member

koic commented Sep 6, 2023

Thanks!

@joergschiller joergschiller deleted the database_from_env_bulk_change_table branch September 6, 2023 18:09
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.

None yet

2 participants