-
Notifications
You must be signed in to change notification settings - Fork 291
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
Improve template to handle empty group labels #2794
Conversation
{% if severity %} | ||
{%- set severity_emoji = {"critical": ":rotating_light:", "warning": ":warning:" }[severity] | default(":question:") -%} | ||
Severity: {{ severity }} {{ severity_emoji }} | ||
{% endif %} | ||
|
||
{%- set status = payload.status | default("Unknown") %} | ||
{%- set status = payload.get("status", "Unknown") %} |
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.
just curious, what is the difference here? I think previous version is more straightforward and clear for non-python developers
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.
Just aligned it with other .get
usages. ( It was used in previous templates also)
{%- set status_emoji = {"firing": ":fire:", "resolved": ":white_check_mark:"}[status] | default(":warning:") %} | ||
Status: {{ status }} {{ status_emoji }} (on the source) | ||
{% if status == "firing" %} | ||
{% if status == "firing" and payload.numFiring %} |
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.
should we add check for numResolved?
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.
I'm checking the numFiring here to not to render empty ':', since alerts from migrated legacy alertmanager have no numFiring field. I'm still thinking how to do it better.
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.
overall looks good to me, see minor comments
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.
is there any way to add some sort of integration tests for these template changes?
– Fix error with missing "groupLabels" or "alertname"
– Support rendering alerts from migrated legacy alertmanagers