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

Add support for API admin methods #398

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

bobmshannon
Copy link
Contributor

@bobmshannon bobmshannon commented Apr 6, 2018

Fixes #337. Adds support for DeleteSeries, CleanTombstones, and Snapshot as documented here.

@beorn7

@beorn7
Copy link
Member

beorn7 commented Apr 6, 2018

Thanks @bobmshannon !

@gouthamve I guess you are most familiar with those endpoints. Does this make sense this way?
@andrestc You are probably most familiar with the current state of the API client code. Would appreciate your opinion.

u := h.client.URL(epCleanTombstones, nil)
q := u.Query()

u.RawQuery = q.Encode()
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to encode anything, I guess. You can send an empty body.

@bobmshannon
Copy link
Contributor Author

bobmshannon commented Apr 6, 2018

Thanks -- I've made some revisions after looking over the code a bit more and the previous PR that originally added this.

@andrestc
Copy link
Contributor

andrestc commented Apr 7, 2018

I think in #291 (comment) we decided to have a single type holding the API for v1. But that was before we had /admin/ endpoints, so I'm not sure if that still applies (if it does, we should add those methods to the API interface, instead of adding an AdminAPI interface). But If we expect this API to keep growing, we could probably keep the PR as is.

Just thought I should've brought that up. Either way, this LGTM.

@bobmshannon
Copy link
Contributor Author

Thanks for the context. I think the single interface is preferable otherwise users will need to instantiate multiple API clients, e.g. one to access the admin methods, another for the status methods, etc. Looking at the previous PR I had assumed that a decision had been made otherwise somewhere along the line. Let me update the PR.

Copy link
Member

@simonpasquier simonpasquier 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 except for 2 small nits.

return err
}

_, _, err = h.client.Do(ctx, req)
Copy link
Member

Choose a reason for hiding this comment

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

L297-302 can be shorten:

_, _, err = h.client.Do(ctx, req)
return err

return err
}

if _, _, err = h.client.Do(ctx, req); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@beorn7
Copy link
Member

beorn7 commented Apr 9, 2018

I agree with @simonpasquier about the nits. @bobmshannon let's fix them. Once done, I'll merge (provided there are no further objections). Thanks, everybody, for chiming in.

@bobmshannon
Copy link
Contributor Author

@beorn7 @simonpasquier I've addressed the PR comments.

@beorn7
Copy link
Member

beorn7 commented Apr 9, 2018

Thank you very much. However, I'm terribly sorry that we have just added another hoop to jump through: We need the DCO sign-off (see recent post on prometheus-developers@). Perhaps, while adding the sign-off, squash the commits into one (which you can then sign off).
Thanks again.

@beorn7
Copy link
Member

beorn7 commented Apr 9, 2018

And of course, the Details link above next to the failed DCO test explains it, too.

@bobmshannon bobmshannon force-pushed the feature/admin_api branch 3 times, most recently from ab88a61 to 633eb73 Compare April 10, 2018 04:03
Signed-off-by: Bob Shannon <bshannon@palantir.com>
@bobmshannon
Copy link
Contributor Author

@beorn7 Okay -- should be signed off now and all status checks are now passing.

@beorn7
Copy link
Member

beorn7 commented Apr 10, 2018

Thanks again, everyone!

@beorn7 beorn7 merged commit e11c6ff into prometheus:master Apr 10, 2018
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.

5 participants