Skip to content
This repository was archived by the owner on May 29, 2024. It is now read-only.

Add alert routing #149

Merged
merged 24 commits into from
Aug 24, 2023
Merged

Add alert routing #149

merged 24 commits into from
Aug 24, 2023

Conversation

adrain-cb
Copy link
Collaborator

@adrain-cb adrain-cb commented Aug 11, 2023

Fixes Issue

Fixes #140

Changes proposed

Screenshots (Optional)

Note to reviewers

Copy link
Collaborator

@ethenotethan ethenotethan left a 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

@adrain-cb adrain-cb requested a review from ethenotethan August 21, 2023 23:11
@adrain-cb adrain-cb marked this pull request as ready for review August 22, 2023 00:25
@adrain-cb adrain-cb changed the title Add alert config Add alert routing Aug 22, 2023
Copy link
Collaborator

@ethenotethan ethenotethan left a 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!

@ethenotethan
Copy link
Collaborator

Can we also update the alerting documentation to reflect this new abstraction within its architecture diagrams and descriptions?

@adrain-cb adrain-cb requested a review from ethenotethan August 23, 2023 18:35
Copy link
Collaborator

@ethenotethan ethenotethan left a 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!

Copy link
Collaborator

@ethenotethan ethenotethan left a comment

Choose a reason for hiding this comment

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

LGTM

@adrain-cb adrain-cb merged commit 63104d3 into master Aug 24, 2023
@adrain-cb adrain-cb deleted the add-alert-config branch August 24, 2023 21:05
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable Alerting Definition
2 participants