-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement POST with get fallback for Query/QueryRange #557
Conversation
Thanks There are few other places where get is used, should these be updated as well? do we have a test that ensures this fallback? |
eed2854
to
2d0ac02
Compare
This does a POST then a GET, so we only want to add this to endpoints we expect POST to work, do we have a list of the ones we want to support fallback on? The change is relatively small, so its easy to add in. As requested in the issue this does simply do a POST and fallback to a GET, there is no control on the client side which method you will use-- this also means if your server only supports GET you'll do a POST then a GET.
From reading the tests, it doesn't seem that anything that actually does this integration test. There are some hard-coded tests that check, but that assumes that the test defines it properly. So I have added a test that the fallback happens as we expect-- which should be sufficient. |
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.
Thanks, just few small nits.
222b53e
to
62dc6e2
Compare
@krasi-georgiev nits addressed :) |
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.
LGTM after adding all full stops.
@juliusv any notes from you before merging?
bummer, failing tests for the different golang versions |
should also run |
failing test because of the need to support older golang versions.
|
httptest.Client has been added in 1.9. If it avoids undue trouble, we could raise the minimum requirement to Go1.9, as we are at Go1.12 already (supporting three versions ought to be enough for everyone). The 1.7 support we still have is essentially because we can. |
Personally I'd rather bump the required version. But I'll leave that decision up to you maintainers :) |
yep I think what @beorn7 suggest is ok so can do it as part of this PR or separate one with some details why it was needed. |
I'll prepare a PR. (We can weed out a bunch of code while we are on it.) |
See #561 . |
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com> Fixes prometheus#428
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
@beorn7 @krasi-georgiev rebased with go version change, CI is passing now. |
Thanks! |
* Implement POST with get fallback for Query/QueryRange Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
Fixes #428