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

Notification emails merging and localization #798

Merged

Conversation

sermakov-orion
Copy link
Contributor

@sermakov-orion sermakov-orion commented Jan 12, 2023

Hi, I would like to introduce couple email notification enhancements.

Why do we need these enhancements?

ovirt-engine has a separate service ovirt-engine-notifier. This service monitors events at the audit_log table and sends the notifications to different recipients (amqp, snmp, and smtp).
We are mostly interested in email notifications about engine backup events (ENGINE_BACKUP_STARTED, ENGINE_BACKUP_FAILED, ENGINE_BACKUP_COMPLETED).
So, first, when the engine-backup.sh script notifies ovirt-engine it sends multiple events in a row for each backup scope (files, db, dwhdb, cinderlib, grafanadb). Finally, If you start one backup then email recipients would receive 10 email messages (5 messages (one per scope item) about the backup started and 5 (one per scope item) messages about the backup completed or failed). We want to combine these messages in a way where the recipient receives only two emails: one for the "backup started" (with all scope items inside) and one for the "backup completed/failed" (with all scope items inside).
And second, currently, the template of the email messages is hard coded. And it is always in English. We need to provide the ability to localize the email contents to other languages.

What did we propose in this change?

For both topics explained above (email merging and email localization) we introduced two additional parameters (see changes at ovirt-engine-notifier.in). By default, both these parameters are empty, so the system would work as it is - nothing would be changed in the current behavior.
But if you specify a list of "log type names" for the MAIL_MERGE_LOG_TYPES parameter then the new "email merging" logic would be activated for the events of specified types.
If you specify some locale code (currently "en" or "ru" are supported, but welcome to help us to provide translations for other languages) for the MAIL_LOCALE parameter then the static fields of the notification email subject (such as "Time: ", "Message: ", etc) would be translated to the specified language.

@sandrobonazzola
Copy link
Member

/ost

@michalskrivanek
Copy link
Member

/ost

Copy link
Member

@michalskrivanek michalskrivanek left a comment

Choose a reason for hiding this comment

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

just a casual review, but it has tests and if automation passes it LGTM

@michalskrivanek
Copy link
Member

/ost

2 similar comments
@michalskrivanek
Copy link
Member

/ost

@michalskrivanek
Copy link
Member

/ost

@sermakov-orion sermakov-orion force-pushed the email-notification-enhancements branch from 76a07d3 to fca4703 Compare May 17, 2023 12:16
@sermakov-orion
Copy link
Contributor Author

/ost

@michalskrivanek
Copy link
Member

lgtm except the properties file that needs to be done in zanata

Copy link
Member

@michalskrivanek michalskrivanek left a comment

Choose a reason for hiding this comment

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

approval just for the sake of OST

@michalskrivanek
Copy link
Member

/ost

@michalskrivanek
Copy link
Member

/ost

eh, builds need to finish first:/

@michalskrivanek
Copy link
Member

...and for that it needs ansible fixes(or at elst disabling the linting) first

Signed-off-by: Stepan Ermakov <sermakov@orionsoft.ru>
@sermakov-orion sermakov-orion force-pushed the email-notification-enhancements branch from fca4703 to 6db8b10 Compare June 5, 2023 05:12
Copy link
Member

@michalskrivanek michalskrivanek left a comment

Choose a reason for hiding this comment

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

approve for OST

@michalskrivanek
Copy link
Member

/ost

2 similar comments
@michalskrivanek
Copy link
Member

/ost

@michalskrivanek
Copy link
Member

/ost

Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

+1

@mwperina mwperina merged commit 193cfcc into oVirt:master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants