-
Notifications
You must be signed in to change notification settings - Fork 92
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(alerts): add support for titleTemplate
to NRQL alert conditions
#1141
feat(alerts): add support for titleTemplate
to NRQL alert conditions
#1141
Conversation
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.
See inline comment
pkg/alerts/nrql_conditions.go
Outdated
@@ -273,6 +275,7 @@ type NrqlConditionUpdateBase struct { | |||
ViolationTimeLimitSeconds int `json:"violationTimeLimitSeconds,omitempty"` | |||
Expiration *AlertsNrqlConditionExpiration `json:"expiration,omitempty"` | |||
Signal *AlertsNrqlConditionUpdateSignal `json:"signal"` | |||
IncidentTitleTemplate *string `json:"incidentTitleTemplate,omitempty"` |
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.
@baNROne I think we'll want to leave off omitempty
for NrqlConditionUpdateBase
. The reason is because we want a user to be able to remove an incident title template on an existing condition that already has the field configured. This presentation (slides 20-25 particularly) does a great job at explaining the reasoning. https://docs.google.com/presentation/d/1VEYfI7acg8LH_uGHjj5Su4RoF1lQRPVyfxK59xUMxV8/edit?usp=sharing
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.
Makes sense. Thanks @akane0915. I've updated it.
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.
Looks good. Manually tested with the terraform PR.
817de1a
to
03c6197
Compare
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.
Code looks good, but let's be sure to manually test once the changes are made in the backend and AGQL to update to titleTemplate
fix: Remove omitempty refactor(alerts): Change field name
03c6197
to
df71d2d
Compare
titleTemplate
to NRQL alert conditions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1141 +/- ##
==========================================
- Coverage 38.84% 38.40% -0.44%
==========================================
Files 86 96 +10
Lines 5612 4822 -790
==========================================
- Hits 2180 1852 -328
+ Misses 3266 2795 -471
- Partials 166 175 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
https://new-relic.atlassian.net/browse/NR-74508
Adds support for
titleTemplate
Cleaned up some formatting while I was in there
See this PR for related changes in the Terraform provider: feat(alerts): Add incident title template support terraform-provider-newrelic#2662
Please do not merge yet