feat: add rocketchat notifier #3600
Conversation
TheMeier
force-pushed
the
rocketchat
branch
6 times, most recently
from
90e581d
to
11915f0
Compare
There was a problem hiding this comment.
I'm not a maintainer but in general looks good! You might have to remove use of the SDK as 1. these tend to be quite large and 2. all of the other integrations avoid using their respective SDKs.
However, I'd wait until @simonpasquier or @gotjosh review before making further changes as I don't want to give you incorrect advice! 🙂
@grobinson-grafana thanks for your feedback. |
TheMeier
force-pushed
the
rocketchat
branch
5 times, most recently
from
d6a5e1c
to
2e16289
Compare
|
I removed the rocketchat SDK. Do you prefer that i squash the commits in this PR? |
TheMeier
force-pushed
the
rocketchat
branch
2 times, most recently
from
e44a344
to
ed5d734
Compare
TheMeier
force-pushed
the
rocketchat
branch
2 times, most recently
from
7614223
to
e3653ad
Compare
TheMeier
force-pushed
the
rocketchat
branch
4 times, most recently
from
7139615
to
37df9c1
Compare
TheMeier
force-pushed
the
rocketchat
branch
2 times, most recently
from
93f9014
to
a507e77
Compare
TheMeier
force-pushed
the
rocketchat
branch
2 times, most recently
from
4e86183
to
c4a7876
Compare
|
@roidelapluie I am aware you have a lot on your plate. I just wanted to kindly ask if there is any chance of this getting some love, since you mentioned some interest in the referenced issue. |
|
@gotjosh can I get a review please? |
TheMeier
force-pushed
the
rocketchat
branch
5 times, most recently
from
c295ec1
to
6255464
Compare
|
@TheMeier I started to look at this, but I'm not sure how to test this. I've run into a number of issues getting RocketChat to run on Apple M1 as it seems there are no docker images for Have you been able to test these changes? |
|
@grobinson-grafana I have tested them with a real-live inhouse rocketchat server. |
There was a problem hiding this comment.
I did a review, just a couple of changes. I would still like to be able to test it too before I approve. @gotjosh how would we best go about getting a trial of RocketChat SaaS?
| if err := json.NewEncoder(&buf).Encode(body); err != nil { | ||
| return false, err | ||
| } | ||
| req, err := http.NewRequest("POST", fmt.Sprintf("%s/%s", n.conf.APIURL.String(), "api/v1/chat.postMessage"), &buf) |
There was a problem hiding this comment.
You should use the postJSONFunc like in https://github.com/prometheus/alertmanager/blob/main/notify/slack/slack.go#L208. I think we can update it to accept headers or even *http.Request?
There was a problem hiding this comment.
Can you please point me at how to do this correctly? notify.PostJSON is used by some integrations but not all. Do you suggest I should change the function signature of notify.PostJSON, and thus change notify.post and notify.request?
There was a problem hiding this comment.
I was thinking of adding a headers argument which would be of the type http.Headers. Let me check to see how big of a change it is, as I might do it in a separate PR which you can then rebase.
There was a problem hiding this comment.
I was tinkering with exactly that yesterday evening and to me it looked like a very substantial change, thus the question.
There was a problem hiding this comment.
This is what I thought #3776.
There was a problem hiding this comment.
jep looks pretty much how I expected/encountered on tinkering
TheMeier
force-pushed
the
rocketchat
branch
2 times, most recently
from
f95adf0
to
e37a96a
Compare
Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
This adds native support for Rocketchat notifications. It uses the Rocketchat REST API. For authentication access tokens are used. This is a solution for #3546