-
Notifications
You must be signed in to change notification settings - Fork 435
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.
Let's also ensure to add:
- Unit tests for new/changed code
- Updated E2E tests to ensure proper config ingestion to alerting flow
- Updated documentation detailing how to use this feature
- A template file that users can copy and quickly fill in
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.
Generally looks a lot better and is certainly shaping up. Think we'll be smooth sailing the review round after this!
Can we also update the alerting documentation to reflect this new abstraction within its architecture diagrams and descriptions? |
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 a few knits and this should be good to go. This looks really good, thank you!
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.
LGTM
Fixes Issue
Fixes #140
Changes proposed
Screenshots (Optional)
Note to reviewers