Fixes #16159 - Rename mail alerts to not be Puppet specific #3741
Conversation
| @@ -0,0 +1,10 @@ | |||
| class ConfigError < MailNotification | |||
There was a problem hiding this comment.
Since this is missing namespace, I'd prefer something more specific. This is too generic term that can be defined in our dependencies. How about ConfigManagementError or similar?
tbrisker
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@lzap Thanks for taking a look - I've updated this according to your suggestions, except for removing the fixture which involves changing most of the mail notifications test. |
|
@lzap ping? |
| puppet_summary = MailNotification.find_by_name('puppet_summary') | ||
| if puppet_summary.present? | ||
| puppet_summary.update_attributes(:name => 'config_summary') | ||
| end |
There was a problem hiding this comment.
MailNotification.where(:name => 'puppet_summary').update_all(:name => 'config_summary')?
| if puppet_error_state.present? | ||
| puppet_error_state.update_attributes(:name => 'config_error_state', | ||
| :type => ConfigManagementError) | ||
| end |
There was a problem hiding this comment.
MailNotification.where(:name => 'puppet_error_state').update_all(:name => 'config_error_state', :type => ConfigManagementError)?
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@tbrisker Updated the migrations as you suggested, thanks 😄 |
|
@tbrisker Rebased - let me know if there is anything else to fix, thanks! |
| MailNotification.where(:name => 'config_summary'). | ||
| update_all(:name => 'puppet_summary') | ||
| MailNotification.where(:name => 'config_error_state'). | ||
| update_all(:name => 'puppet_error_state', :type => ConfigManagementError) |
There was a problem hiding this comment.
shouldn't this be type => PuppetError?
Right now, under 'my account', users get emails with alerts about the status of their hosts regarding configuration management (e.g: if there was an error, get an alert, or get a weekly summary of all hosts changes) These emails gather info from reports that could come from Puppet, Ansible, Salt or Chef, but there are references to Puppet all over them. I think these Puppet references should be removed as these reports may not come from Puppet.
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@tbrisker Updated to fix the problem in the |
|
Thanks @dLobatog ! |
Right now, under 'my account', users get emails with alerts about the
status of their hosts regarding configuration management (e.g: if there
was an error, get an alert, or get a weekly summary of all hosts
changes)
These emails gather info from reports that could come from Puppet,
Ansible, Salt or Chef, but there are references to Puppet all over
them.
I think these Puppet references should be removed as these reports
may not come from Puppet.
Another step further in this direction would be to start storing a
'origin' or similar field in Reports and Facts that indicates where
they come from.