Skip to content
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

*: remove v1/alerts/groups API endpoint #1525

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

simonpasquier
Copy link
Member

Closes #1508

I had this change for a while in my backlog, just rebased on master to fix some conflicts.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the thorough cleanup. Just to ensure, that we are mentioning this in the release notes, would you please add an entry to the Next release section of CHANGELOG.md in this PR?

@@ -71,6 +71,15 @@ var corsHeaders = map[string]string{
"Cache-Control": "no-cache, no-store, must-revalidate",
}

// Alert is the API representation of an alert, which is a regular alert
// annotated with silencing and inhibition info.
type Alert struct {
Copy link
Member

Choose a reason for hiding this comment

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

Great! Thanks for moving APIAlert into api/api.go.

@simonpasquier
Copy link
Member Author

@mxinden done!

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

alertSlice() on dispatch.go#246 can also be removed, then this is good to go

@mxinden
Copy link
Member

mxinden commented Aug 22, 2018

In case one of you clicks merge first, would you mind squashing? Makes preparing the CHANGELOG.md easier for the next release.

@stuartnelson3
Copy link
Contributor

squash4lyfe

@mxinden
Copy link
Member

mxinden commented Aug 23, 2018

@simonpasquier would you mind resolving the conflict?

Signed-off-by: Simon Pasquier <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>
@simonpasquier simonpasquier force-pushed the remove-group-alert-api branch from 789fe15 to a7738d2 Compare August 23, 2018 12:20
@simonpasquier
Copy link
Member Author

@mxinden done

@mxinden mxinden merged commit 899226f into prometheus:master Aug 23, 2018
@mxinden
Copy link
Member

mxinden commented Aug 23, 2018

Thanks @simonpasquier!

@mpursley
Copy link

mpursley commented Feb 15, 2019

Hi @simonpasquier @mxinden, sounds like removing the "v1/alerts/groups" endpoint from the V1 API has broken compatibility with some external clients.. e.g.
"Don't work with the latest version api (v2) alertmanager" - prymitive/karma#115
"Incompatibility with alertmanager 0.16.x" - cloudflare/unsee#273

I wonder if it make sense to just put api/v1/alerts/groups back in? (E.g. just re-add the code that was removed from this PR?)

As:

  1. This is the only endpoint from the V1 API that was removed, and
  2. It sounds like this endpoint was just removed for reasons like "this is a size-able chunk of code that probably hasn't been used for over a year". Though, there are some remote clients (like Unsee and Karma) that are using this endpoint, so it might be worth putting it back into the V1 API, to continue to support these API clients?

@roidelapluie
Copy link
Member

Yes +1 for adding it back. We use karma.

@simonpasquier
Copy link
Member Author

@mpursley sorry for the pain, if I would have known that that endpoint was used, it wouldn't have been removed probably. The v1 API is deprecated and the plan is to implement something similar with the v2 API to accommodate karma (see #868).

@mpursley
Copy link

@mpursley sorry for the pain, if I would have known that that endpoint was used, it wouldn't have been removed probably. The v1 API is deprecated and the plan is to implement something similar with the v2 API to accommodate karma (see #868).

Yes, understood. It looks like #868 is now updated to "P2", so hopefully that update/fix gets added soon(ish). Just keep in mind that there will be users out there that are not able to update unsee/karma or alertmanager until this endpoint gets returned or replaced.

Thx @simonpasquier

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

Successfully merging this pull request may close these issues.

Remove alert_groups from alertmanager
5 participants