-
Notifications
You must be signed in to change notification settings - Fork 21
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
Richer alerts; improved docs; easier testing #39
base: master
Are you sure you want to change the base?
Conversation
We only need the src folder in the build container, not all the other cruft.
New logging module with configurable log level.
Alerts now: - link to the alertmanager URL and a dashboard if env vars are configured. - all labels other than explicitly formatted labels are included in the alert output - when annotation.url is set the link will be included in the alert - fixed extra spaces that broke the alert link - emit the alert severity instead of "firing" when available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW these all seem like sensible improvements to me. They're orthogonal though, each commit could be it's own PR and be merged independently.
Co-authored-by: Jelmer Vernooij <jelmer.vernooij@aiven.io>
@jelmer - yes - I intentionally make commits each with their own goal - so you're right they could be their own PRs. If you merge without squashing they will continue to be discrete changes in the history. As they're all applied on my forked master branch - I guess the alternative would be to cherry pick each commit into it's own branch and raise multiple PRs - but the end result would be the same. |
@jinnko makes sense - FWIM I'm not a committer to matrix-alertmanager, just an interested contributor |
You may not want to merge this, but thought I'd share just in case.
From the individual commit message:
configured.
alert output