Add parameter of ProxyConnectHeader #145
Conversation
goreq.go
Outdated
|
|
||
| func (r *Request) AddProxyHeader(name string, value string) { |
There was a problem hiding this comment.
Maybe change this to AddProxyConnect header? Because these headers will be only sent during the connect phase according to the golang doc. Same for WithProxyHeader
There was a problem hiding this comment.
ok.
Change the method name.
goreq_test.go
Outdated
| @@ -1065,6 +1066,98 @@ func TestRequest(t *testing.T) { | |||
|
|
|||
| }) | |||
|
|
|||
| g.Describe("TLS Proxy", func() { | |||
There was a problem hiding this comment.
This test seem a bit overcomplicated. You just need to verify that the header gets passed to the proxy. No need to implement a whole proxy in the test.
There was a problem hiding this comment.
Since I could not think of a simple test, I am wondering if there is no problem as a test.
There was a problem hiding this comment.
@kilisima what I'm saying is that you don't need to implement a whole proxy. You can safely remove the TLS server, and then do the assertion in the httptest server. No need to dial to a new server. You just need to assert that the headers arrive in the proxyCh which you're already doing.
There was a problem hiding this comment.
I removed the TLS server from the test case and simplified the test case.
Thank you. I appreciate it.
There was a problem hiding this comment.
@kilisima thank you for doing that. After reviewing the PR again I realize that there's no need for a Describe("TLS Proxy") block as we're not testing anything related to TLS here. I think it's best if we remove that describe and we move the test to the Proxy describe block and re-use the http test server that's being used there which makes sense. That way everything will be consistent and even simpler.
Sorry to bring this up this late, It's that I just realized about that.
There was a problem hiding this comment.
Fixed. Please check.
goreq_test.go
Outdated
| proxyCh <- r | ||
| // Implement an entire CONNECT proxy | ||
| if r.Method == "CONNECT" { | ||
| // hijacker, ok := w.(http.Hijacker) |
There was a problem hiding this comment.
No need to leave it commented you can go ahead and remove it.
There was a problem hiding this comment.
Thank you, I deleted it.
|
@marcosnils Thanks to you I was helped. Please merge. |
Correspondence to parameters added in Golang 1.9
I want to assign parameters to the header for authentication to the Proxy
golang/go#19504