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

feat: refactoring of alerts and send correct email alerts #1788

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

tboerger
Copy link
Collaborator

@tboerger tboerger commented Feb 27, 2024

Previously the sent email alerts have been missing mandatory headers like Date and it was also missing content type, content transfer encoding and mime version. I have taken proper examples form the unmaintained gomail library to build right emails.

Besides that I have refactored the calls for alerts, they git the same structure now and it should be prepared to inject custom templates for all altering methods at some later point. Generally it is prepared for a more flexible alert handling.

Fixes #1782
Fixes #1627
Fixes #1571

Replaces #1771
Replaces #1636

@samerbahri98
Copy link
Contributor

I am not convinced this is a refactor for alerts. As the main issues with the initial alerts implementation (being tightly coupled to the currently supported alerts, making it very hard to support any other alerting mechanism for now) are still there. This change should be considered as a bug fix and not a refactor (ie: maybe the title and the description of this pull request should be slightly changed).

That being said, I still think that this pull request should be reviewed and merged, for the purpose of fixing the mentioned bugs, and to serve as a functional starting point for a refactor. Either way, I am not a maintainer, so make of my opinion what you want.

@tboerger
Copy link
Collaborator Author

tboerger commented Mar 2, 2024

The code have been still refactored, not just fixed. Is it more flexible than before? No, not yet.

@fiftin
Copy link
Collaborator

fiftin commented Mar 2, 2024

@tboerger did you test it?

@tboerger
Copy link
Collaborator Author

tboerger commented Mar 2, 2024

I have tested the mail alerts with mailhog as a target and it contains all the mandatory fields like To, Date and so on.

@tboerger
Copy link
Collaborator Author

tboerger commented Mar 2, 2024

Looks like I got to resolve conflicts anyway as the teams integration have been merged.

Previously the sent email alerts have been missing mandatory headers
like `Date` and it was also missing content type, content transfer
encoding and mime version. I have taken proper examples form the
unmaintained gomail library to build right emails.

Besides that I have refactored the calls for alerts, they git the same
structure now and it should be prepared to inject custom templates for
all altering methods at some later point. Generally it is prepared for a
more flexible alert handling.
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.

Email header incomplete Slack notifications not working Email alerts for docker images don't work anymore
3 participants