-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Actually set HTTP version in response #2969
Conversation
Version is worth special-casing here, since the build information is pretty important, and make components need it. Fixes issue #2958.
|
@otoolep Lets actually add a test this time :-). |
7f9358d
to
54eaa74
Compare
Unit test added @corylanou |
54eaa74
to
9255e00
Compare
9255e00
to
0131956
Compare
+1 |
Actually set HTTP version in response
|
||
resp, _ := http.Get(s.URL() + "/query") | ||
got := resp.Header.Get("X-Influxdb-Version") | ||
if version != version { |
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.
this test will never fail 😉
if got != version
thanks for the fix @otoolep - my PR was a drive-by fix, sorry about that. since now you've looked at this same code, i was wondering what you thought about putting the value for the X-Influxdb-Version header in the config? by default it could be based on the build version but i think it could also be a feature that the user could set it themselves to some arbitrary value. each of the constructors that needed the version already had the config and it would make it simpler. |
I thought about the config @neonstalwart but decided to leave the config just for actual config stuff. |
fair enough - that's the way i leaned too. |
The HTTP version was being set using information in the Server, but the Server's version wasn't set at the time. Move version into
NewServer
so it's now required.Version is worth special-casing here, since the build information is pretty important, and many components need it.