-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: add grafana on call as chat provider #4759
Conversation
5d168f1
to
4ec338c
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.
just a few questions and changes
@@ -0,0 +1,78 @@ | |||
{ | |||
"name": "@novu/grafana-on-call", | |||
"version": "0.16.3", |
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.
This version should be the same as the rest of the providers, v0.21.0
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.
okay, out of curiocity, whats the reason to keep all provider version same? also this is auto generated.
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 am not sure, that is a @LetItRock question
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.
upgraded to 0.21.0
const { headers } = await this.axiosInstance.post(url.toString(), body); | ||
|
||
return { | ||
id: this.config.alertUid || options.content, |
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.
What does the options.content return?
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.
this is actually the message itself, there is no valid identifier that grafana returns in response(body/header), it it acceptable to generate a uuid and use it as it instead?
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.
For this use-case yes, we should not be using the actual message as the id here.
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.
done
4e6113c
to
f66fb82
Compare
apps/web/public/static/images/providers/dark/square/grafana-on-call.svg
Outdated
Show resolved
Hide resolved
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, just need to fix the icon.
f66fb82
to
a6bf5f7
Compare
What change does this PR introduce?
Why was this change needed?
closes #4682 and #4438
Other information (Screenshots)
Integration config
Workflow
Grafana dashboard