gh: Fork compatibility fix #535
Conversation
| @@ -122,7 +122,7 @@ Scenario: Related fork already exists | |||
| Given the GitHub API server: | |||
| """ | |||
| before { halt 401 unless request.env['HTTP_AUTHORIZATION'] == 'token FITOKEN' } | |||
| post('/api/v3/repos/evilchelu/dotfiles/forks', :host_name => 'git.my.org') { '' } | |||
There was a problem hiding this comment.
This gives me a sad. I envisioned that HUB_TEST_HOST acts as an override for the hostname to connect to but to also keep the original hostname in the Host header so that test can verify that the correct host was used. We now lose the ability to assert the correct hostname
There was a problem hiding this comment.
Forgot to clarify a bit why this is removed. My bad.
In Go, it's not possible to override the Host header on Request. What I observed was I set the Host header before sending the request and on the test server it receives nothing for this header. On the other hand, we also sort of test the host name in the assertion as well, e.g., https://github.com/github/hub/blob/fork_compat/features/fork.feature#L131.
An alternative to solve this problem is set a custom header and assert it in the test server, say X-HUB-TEST-HOST.
There was a problem hiding this comment.
I'm guessing Go tries to enforce the Host header to be the same as the URL host passed into Request.Host.
I could make it work if we want to continue this approach for testing the header. We could just use a non-reserved header like X-HUB-TEST-HOST.
/cc @technoweenie since he may know the answer.
There was a problem hiding this comment.
I don't. It sounds like a bug. I would expect something like curl <url> -H "Host: <other-host>" to work in Go.
There was a problem hiding this comment.
Figured out a workaround:
client := &http.Client {}
req, _ := http.NewRequest("GET", "http://127.0.0.1:9292", nil)
// req.Header.Add("Host", "example.com") //=> shit doesn't work
req.Host = "example.com"
client.Do(req)There was a problem hiding this comment.
👍 👏
Will pull back the assertion from the cukes
There was a problem hiding this comment.
@mislav I tried to set the host like you did, req.Host = "example.com". The server is still getting empty Host header. Is there anything I missed?
There was a problem hiding this comment.
It's hard to believe that the Host header is literally empty; every HTTP request should specify the Host value. Are you using a HTTP debugging proxy to debug this?
Maybe req.Host is getting reset somewhere in Octokit?
Filed a bug, BTW https://code.google.com/p/go/issues/detail?id=7682
There was a problem hiding this comment.
This is a simplified version of what I'm doing: http://play.golang.org/p/5I0i3S_szO:
There was a problem hiding this comment.
@mislav I figured out the problem. Host doesn't show up in Request.Header but in Request.Host. See the comment for Request.Host:
// The host on which the URL is sought.
// Per RFC 2616, this is either the value of the Host: header
// or the host name given in the URL itself.
// It may be of the form "host:port".
A modification of the example gets this to work: http://play.golang.org/p/DEFLb7_WRJ.
req.Header.Add("Host", "example.com") still doesn't work...
|
@mislav testing host name is now available with 6ab839f. And I put back the filters: 3c75808. 😸 I'll need to wait for the merge of octokit/go-octokit#60 in |
|
Nice 👍 |
All
features/fork.featureis passing now.