Fix notification missing substituted values, log deprecation if other places do this #20978
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrafanie
commented
Jan 21, 2021
app/models/notification.rb
Outdated
| # 1. Deprecate now | ||
| # 2. Fail validation going forward via errors.add(error_args) | ||
| error_args = [:options, "text bindings for notification_type: '#{notification_type.name}' failed with error: '#{e.message}' with options: '#{options.inspect}' and message #{notification_type.message.inspect}. Next release will not allow a notification without complete bindings."] | ||
| ActiveSupport::Deprecation.warn("#{error_args.join(' ')}", caller_locations[1..-1].reject {|location| location.label.include?("emit_for_event")}) |
There was a problem hiding this comment.
Note, emit_for_event above is the primary caller where notifications get created indirectly if the event has a matching notification_type when you raise an event so we have to skip it as a possible caller since that's not where you need to fix the code to provide a complete event, including the full_data containing the values to substitute. It's complicated, but that's how it works currently.
There was a problem hiding this comment.
I would vote to put some sort of comment in the code directly about it then so the reasoning isn't lost to time.
…y_exceeded event Fixes ManageIQ#20977
Fixes ManageIQ#20977 Fixes warnings of the variety: DEPRECATION WARNING: options text bindings for notification_type: automate_service_provisioned failed with error: 'key{subject} not found' with options: '{}' and message "Service %{subject} has been provisioned.". Next release will not allow a notification without complete bindings. (called from block (3 levels) in <top (required)> at /Users/joerafaniello/Code/manageiq/spec/models/notification_spec.rb:14) DEPRECATION WARNING: options text bindings for notification_type: automate_service_provisioned failed with error: 'key{subject} not found' with options: '{}' and message "Service %{subject} has been provisioned.". Next release will not allow a notification without complete bindings. (called from block (3 levels) in <top (required)> at /Users/joerafaniello/Code/manageiq/spec/models/notification_spec.rb:15) DEPRECATION WARNING: options text bindings for notification_type: service_retired failed with error: 'key{subject} not found' with options: '{}' and message "Service %{subject} has been retired.". Next release will not allow a notification without complete bindings. (called from block (3 levels) in <top (required)> at /Users/joerafaniello/Code/manageiq/spec/models/notification_spec.rb:16) DEPRECATION WARNING: options text bindings for notification_type: automate_tenant_info failed with error: 'key{message} not found' with options: '{}' and message "%{message}". Next release will not allow a notification without complete bindings. (called from block (5 levels) in <top (required)> at /Users/joerafaniello/Code/manageiq/spec/models/notification_spec.rb:98)
jrafanie
force-pushed
the
fix_notification_missing_substituted_values
branch
from
5fecc07
to
8e87dc4
Compare
|
Checked commits jrafanie/manageiq@62847b3~...8e87dc4 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint app/models/notification.rb
|
simaishi
pushed a commit
that referenced
this pull request
Jan 29, 2021
…tituted_values Fix notification missing substituted values, log deprecation if other places do this (cherry picked from commit cac4c50)
|
Kasparov backport details: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #20977
https://talk.manageiq.org/t/how-can-i-solve-this-issue-the-server-name-memory-usage-memory-usage-exceeded-limit-memory-threshold-the-server-process-pid-exited-and-will-be-restarted/5261
cc @djberg96 @himdel (thanks Martin for the deep dive)