-
Notifications
You must be signed in to change notification settings - Fork 133
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
Add alertmanager provider #258
Conversation
10713de
to
6d637c9
Compare
169f4a0
to
5ed5de8
Compare
Should be good now @stefanprodan apologies for the many rebases again... |
5ed5de8
to
ff0fa4d
Compare
docs/spec/v1beta1/provider.md
Outdated
|
||
| Label | Description | | ||
| ----------- | ------------------------------------------------------------------------------ | | ||
| alertname | The string FluxNotification | |
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.
I think the alertname
should be the controller name set in event.ReportingController
.
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.
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.
I think instead the report controller should be a label inside the alert rather than the alertname.
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.
Hmm ok so maybe the alertname should be the reason? FluxNotification
doesn't seem ok to me.
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.
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.
Flux(Kustomization|HelmRelease)Progressing
looks good to me 💯
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.
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.
Nice! Thanks
ff0fa4d
to
4accc08
Compare
Hmm not sure why DCO is hanging now. |
4accc08
to
fcd7ebc
Compare
tried another push seems to be still be busted... |
The Probot DCO bot was down the whole day... it's back up now, please rebase with upstream and force push. |
This commit adds the alertmanager provider. The provider adds some generic labels based on the event which should be enough to configure appropraite routes within alertmanager. The alert is annotated with the message by default and optionally by the summary field given in the event. Signed-off-by: Alan Hollis <me@alanhollis.com>
fcd7ebc
to
8bf8150
Compare
Done ty |
@stefanprodan just checking that there's nothing more you'd like me to do here? Happy to continue until we can get this merged :) |
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
Thanks @Alan01252 🎖️
This commit adds the alertmanager provider. The provider adds some
generic labels based on the event which should be enough to configure
appropraite routes within alertmanager.
The alert is annotated with the message by default and optionally by the
summary field given in the event.