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

Slack mentions should check length of the rendered template instead of the template source #129

Open
chris-barbour-as opened this issue Oct 11, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@chris-barbour-as
Copy link

The slack user and group mentions should check the length of the rendered go template when deciding whether or not to add a mention to the slack pretext. Currently, the logic checks the length of the source string for it's conditional logic, and renders the template when interpolating it into the slack pretext.

This is a problem, because if the go template renders into an empty string, a broken mention will be added to the pretext.

To reproduce, use {{- /* empty */ -}} as the "Mention Groups" configuration setting.

if len(sn.settings.MentionGroups) > 0 {
appendSpace()
for _, g := range sn.settings.MentionGroups {
mentionsBuilder.WriteString(fmt.Sprintf("<!subteam^%s>", tmpl(g)))
}
}
if len(sn.settings.MentionUsers) > 0 {
appendSpace()
for _, u := range sn.settings.MentionUsers {
mentionsBuilder.WriteString(fmt.Sprintf("<@%s>", tmpl(u)))
}
}

@armandgrillet armandgrillet added the bug Something isn't working label Oct 18, 2023
DGuhr added a commit to DGuhr/alerting that referenced this issue Feb 17, 2024
Just adds the mentions to the slack message when the found value, trimmed for whitespaces, is not an empty string
DGuhr added a commit to DGuhr/alerting that referenced this issue Feb 17, 2024
…ntaining non-empty values

only add to builder when they are not empty.
@DGuhr
Copy link

DGuhr commented Feb 17, 2024

Hey @armandgrillet - For reasons, I checked the codebase a bit, and found this bug. If it is still valid, I have a fix here (I think, if I understood everything correctly): main...DGuhr:alerting:fix_slack_broken_mentions - see tests for details. Happy to provide a PR if you want.

Alternatively, I think, the check could be added to the config_util.go method for comma separated strings, but this is also used elsewhere and I wanted to avoid impacting other behaviour.

edit: on second glance, I saw that splitCommaSeparatedString in config_util does exactly what I did here, so I guess that the bug doesn't exist anymore or I am missing some context currently. sry for the noise so-far.

DGuhr added a commit to DGuhr/alerting that referenced this issue Feb 17, 2024
…ntaining non-empty values

only add to builder when they are not empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

3 participants