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

Actually set HTTP version in response #2969

Merged
merged 4 commits into from
Jun 12, 2015
Merged

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Jun 12, 2015

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.

otoolep added 2 commits June 12, 2015 11:28
Version is worth special-casing here, since the build information is
pretty important, and make components need it.

Fixes issue #2958.
@otoolep
Copy link
Contributor Author

otoolep commented Jun 12, 2015

@neonstalwart @corylanou

@otoolep
Copy link
Contributor Author

otoolep commented Jun 12, 2015

go build -ldflags="-X main.version v1234 -X main.commit xxx"
.
.
.
$ curl -v -G http://localhost:8086/query --data-urlencode "q=CREATE DATABASE foo"
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8086 (#0)
> GET /query?q=CREATE%20DATABASE%20foo HTTP/1.1
> User-Agent: curl/7.35.0
> Host: localhost:8086
> Accept: */*
> 
< HTTP/1.1 200 OK
< Content-Type: application/json
< Request-Id: 21222deb-1133-11e5-8001-000000000000
< X-Influxdb-Version: v1234
< Date: Fri, 12 Jun 2015 18:45:00 GMT
< Content-Length: 49
< 
* Connection #0 to host localhost left intact
{"results":[{"error":"database already exists"}]}

@otoolep otoolep added this to the 0.9.1 milestone Jun 12, 2015
@otoolep otoolep self-assigned this Jun 12, 2015
@corylanou
Copy link
Contributor

@otoolep Lets actually add a test this time :-).

@otoolep otoolep force-pushed the actually_set_http_version branch from 7f9358d to 54eaa74 Compare June 12, 2015 19:24
@otoolep
Copy link
Contributor Author

otoolep commented Jun 12, 2015

Unit test added @corylanou

@otoolep otoolep force-pushed the actually_set_http_version branch from 54eaa74 to 9255e00 Compare June 12, 2015 19:26
@otoolep otoolep force-pushed the actually_set_http_version branch from 9255e00 to 0131956 Compare June 12, 2015 19:31
@corylanou
Copy link
Contributor

+1

otoolep added a commit that referenced this pull request Jun 12, 2015
@otoolep otoolep merged commit f28362d into master Jun 12, 2015
@otoolep otoolep deleted the actually_set_http_version branch June 12, 2015 19:49

resp, _ := http.Get(s.URL() + "/query")
got := resp.Header.Get("X-Influxdb-Version")
if version != version {
Copy link
Contributor

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

@neonstalwart
Copy link
Contributor

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.

@otoolep
Copy link
Contributor Author

otoolep commented Jun 15, 2015

I thought about the config @neonstalwart but decided to leave the config just for actual config stuff.

@neonstalwart
Copy link
Contributor

fair enough - that's the way i leaned too.

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.

3 participants