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: add alert summary to status updates #2074

Merged
merged 16 commits into from
Feb 9, 2022

Conversation

Forfold
Copy link
Contributor

@Forfold Forfold commented Dec 28, 2021

Description:
This PR adds the alert summary to status update messages. Status updates show the alert ID, but do not show the summary or provide a link to the alert, which can make the investigation of an alert cumbersome.

The current alert SMS template is conditionally rendering alert, alert bundle, and alert status messages, so I've made three explicit templates that can be leveraged when setting the type on the alertSMS struct before rendering. This should help with maintenance should we want to change any of the templates again.

Which issue(s) this PR fixes:
Closes #2056

notification/twilio/alertsms.go Outdated Show resolved Hide resolved
notification/twilio/alertsms.go Outdated Show resolved Hide resolved
@dctalbot dctalbot self-requested a review January 10, 2022 18:23
@Forfold Forfold requested review from dctalbot and mastercactapus and removed request for dctalbot and mastercactapus January 26, 2022 20:40
mastercactapus
mastercactapus previously approved these changes Jan 31, 2022
Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

Unable to run make start, looks like there are some unintended changes to go.mod? Got it to work using git checkout master -- go.mod

Also, the spacing is a bit odd now that I see it, I like the separation with the newlines, but the indentation isn't necessary IMO. If we do want to change, could go in a new issue potentially. For reference, here is my UX for an integration I named "web int":

Screen Shot 2022-02-08 at 9 54 38 AM

@Forfold
Copy link
Contributor Author

Forfold commented Feb 8, 2022

@dctalbot what version of Go are you on? I had to update to 1.17.

@dctalbot
Copy link
Contributor

dctalbot commented Feb 8, 2022

@dctalbot what version of Go are you on? I had to update to 1.17.

@Forfold go version go1.17 darwin/amd64

@Forfold
Copy link
Contributor Author

Forfold commented Feb 8, 2022

@dctalbot I see, well it's a result of make generate (see last commits) so it should probably go in

@mastercactapus mastercactapus merged commit 6e76778 into master Feb 9, 2022
@mastercactapus mastercactapus deleted the status-update-add-summary branch February 9, 2022 15:53
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.

Add option to include original summary/details in alert status updates
3 participants