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.

Sign up for GitHub

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

Use rails deprecation behavior but log in production #18847

Merged
merged 2 commits into from
Jun 10, 2019
Merged

Use rails deprecation behavior but log in production #18847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bac06ac
Copy current edge rails defaults for test/dev
jrafanie Jun 10, 2019
201ebfe
Log deprecations in production mode
jrafanie Jun 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/environments/development.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
# Don't care if the mailer can't send
config.action_mailer.raise_delivery_errors = false

# Print deprecation notices to the Rails logger
# Print deprecation notices to the Rails logger.
Copy link
Member

Choose a reason for hiding this comment

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

Man, this line is SUPER important!

Copy link
Member Author

Choose a reason for hiding this comment

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

Man, this line is SUPER important!

Period.

config.active_support.deprecation = :log

# Only use best-standards-support built into browsers
Expand Down
8 changes: 6 additions & 2 deletions config/environments/production.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,12 @@
# the I18n.default_locale when a translation can not be found)
config.i18n.fallbacks = [I18n.default_locale]

# Send deprecation notices to registered listeners
config.active_support.deprecation = :notify
# Send deprecation notices to registered listeners.
# config.active_support.deprecation = :notify
Copy link
Member

Choose a reason for hiding this comment

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

Does a vanilla production.rb have both lines, but one commented out? If not, then I would think we'd just delete this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I left it because the comment is wrong with this line saying :log instead of :notify

Copy link
Member Author

Choose a reason for hiding this comment

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

If I drop the the line that ships with rails, I should drop the comment too. They should stay or go together.


# Change from rails :notify default since it's unlikely we'll have users setup
# notifications for deprecation warnings.
config.active_support.deprecation = :log
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the 🍖 of the PR. The default of notify is great when hosted as you can setup notifications for these. Since most of our users are not hosted by us, it will be unlikely we'll ever get deprecations reported to us unless we get them by default from logging.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure...see my comment below. What does this solve that we couldn't do previously?

Copy link
Member Author

Choose a reason for hiding this comment

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

Deprecations, especially rails ones, should be exceptional. I'd like to stay on top of them so future upgrades are easier. Note, this doesn't affect Vmdb::Deprecation as that will never log in production so this only logs rails deprecations, ones we should definitely stay on top of.

Copy link
Member

@kbrock kbrock Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we have a lot of code suppressing the notifications in production? At least for our own deprecations.

UPDATE: ugh, you just said that


# Log the query plan for queries taking more than this (works
# with SQLite, MySQL, and PostgreSQL)
Expand Down
4 changes: 2 additions & 2 deletions config/environments/test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
config.cache_classes = true
config.eager_load = false

# Print deprecation notices to the stderr
#ActiveSupport::Deprecation.behavior = :stderr
# Print deprecation notices to the stderr.
Copy link
Member

Choose a reason for hiding this comment

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

Again, extremely important!

config.active_support.deprecation = :stderr
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix the syntax for this to the defaults from rails.


# Configure static asset server for tests with Cache-Control for performance
config.public_file_server.enabled = true
Expand Down