Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Remove POST /api/v1/alerts #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

codesome
Copy link
Member

While we are working on https://docs.google.com/document/d/1ExiSoTrIKN_6fsWUj20trbaIXJfFfp--lwTu2mLNzcs/edit#, I have added default_receiver for config and receiver for posting alerts since we won't have the routes in the first iteration.

pkg/api/alertmanager.go Outdated Show resolved Hide resolved
@codesome
Copy link
Member Author

Another update on this PR: we actually (or might not) need POST on /alerts, if so, I will update the PR to remove it altogether

@codesome codesome changed the title Additions to config and postable alerts Additions to config and remove POST to /api/v1/alerts Feb 23, 2021
owen-d
owen-d previously approved these changes Feb 23, 2021
@owen-d
Copy link
Member

owen-d commented Feb 23, 2021

I'm a bit unsure about this one. While I know we don't plan to introduce routing for the first release, I think it'd benefit us to use routing, even if we just create one route per alert. This allows us to model everything in the context of routes so we can introduce them seamlessly later. WDYT?

@buro9
Copy link

buro9 commented Feb 24, 2021

Agree on leaving it, even if you serve a HTTP 501 (NOT IMPLEMENTED) for the time being.

@codesome codesome changed the title Additions to config and remove POST to /api/v1/alerts Add receivers to POST /api/v1/alerts Mar 1, 2021
@codesome
Copy link
Member Author

codesome commented Mar 1, 2021

@owen-d I have updated this PR to add receivers to POST /alerts instead based on the discussion in slack (not converting receivers on rules to routes but instead explicitly pass the receivers while alerting). (Note that this POST is only for reference and not implemented in the first iteration)

@owen-d
Copy link
Member

owen-d commented Mar 1, 2021

I'm not sure this needs to be in the API. Grafana managed alerts will have an alerting channel tied to them initially, but this is in the rule config. I don't think we need to extend the alert endpoint with this field because the only caller would be Grafana itself, which does not need to use an http API, but can instead do this in process. Can't we just leave this out of the POST /api/v1/alerts endpoint entirely?

@codesome
Copy link
Member Author

codesome commented Mar 9, 2021

Can't we just leave this out of the POST /api/v1/alerts endpoint entirely?

Yes, that is the plan, the API is not required to be implemented now. The plan was to have this struct in place so that we can align the interface onto something (so that the rule evaluation knows in what structure to send the alert). If it will cause confusion with the API, I can remove it from here and have the struct only in the notifier code.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome codesome changed the title Add receivers to POST /api/v1/alerts Remove POST /api/v1/alerts Mar 9, 2021
@codesome
Copy link
Member Author

Nudge @owen-d @papagian

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants