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

Implement POST with get fallback for Query/QueryRange #557

Merged
merged 2 commits into from Apr 30, 2019

Conversation

jacksontj
Copy link
Contributor

Fixes #428

@jacksontj
Copy link
Contributor Author

cc @krasi-georgiev

@krasi-georgiev
Copy link
Contributor

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?
I mean ensure that an endpoint that supports only GET uses the GET method and an endpoint that support POST uses the post method.

@jacksontj jacksontj force-pushed the issue_428 branch 3 times, most recently from eed2854 to 2d0ac02 Compare April 24, 2019 17:13
@jacksontj
Copy link
Contributor Author

There are few other places where get is used, should these be updated as well?

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.

do we have a test that ensures this fallback?

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.

Copy link
Contributor

@krasi-georgiev krasi-georgiev left a 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.

api/client.go Outdated Show resolved Hide resolved
api/client_test.go Show resolved Hide resolved
api/client_test.go Outdated Show resolved Hide resolved
@jacksontj jacksontj force-pushed the issue_428 branch 2 times, most recently from 222b53e to 62dc6e2 Compare April 25, 2019 17:56
@jacksontj
Copy link
Contributor Author

@krasi-georgiev nits addressed :)

Copy link
Contributor

@krasi-georgiev krasi-georgiev left a 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?

api/client_test.go Outdated Show resolved Hide resolved
@krasi-georgiev
Copy link
Contributor

bummer, failing tests for the different golang versions

api/client_test.go Outdated Show resolved Hide resolved
@krasi-georgiev
Copy link
Contributor

should also run GO111MODULE=on go mod tidy to update the go.mod file.

@krasi-georgiev
Copy link
Contributor

krasi-georgiev commented Apr 27, 2019

failing test because of the need to support older golang versions.

api/client_test.go:154: server.Client undefined (type *httptest.Server has no field or method Client)

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2019

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.

@jacksontj
Copy link
Contributor Author

Personally I'd rather bump the required version. But I'll leave that decision up to you maintainers :)

@krasi-georgiev
Copy link
Contributor

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.

@beorn7
Copy link
Member

beorn7 commented Apr 28, 2019

I'll prepare a PR. (We can weed out a bunch of code while we are on it.)

@beorn7
Copy link
Member

beorn7 commented Apr 28, 2019

See #561 .

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

Fixes prometheus#428
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
@jacksontj
Copy link
Contributor Author

@beorn7 @krasi-georgiev rebased with go version change, CI is passing now.

@krasi-georgiev krasi-georgiev merged commit a4daf00 into prometheus:master Apr 30, 2019
@krasi-georgiev
Copy link
Contributor

Thanks!

@jacksontj jacksontj deleted the issue_428 branch April 30, 2019 16:24
jacksontj added a commit to jacksontj/client_golang that referenced this pull request Apr 30, 2019
* Implement POST with get fallback for Query/QueryRange

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
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.

API client doesn't support POST method for Query/QueryRange
3 participants