-
Notifications
You must be signed in to change notification settings - Fork 61
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
Changed: add possibility to set timeout, dial and transport #26
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
proxy.go
Outdated
@@ -102,6 +102,21 @@ func NewProxy(config Config) *Proxy { | |||
return proxy | |||
} | |||
|
|||
// SetTimeout set Timeout of the proxy. |
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 is just a read timeout, not a connection timeout, should be explained (or maybe we should rename both the property and the function?)
} | ||
|
||
// SetTransport set RoundTripper of the proxy. | ||
func (p *Proxy) SetTransport(transport *http.Transport) { |
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.
Could you please explain what's the need for exposing transport?
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.
The need is to be able to control the transport configuration (dialer, proxy, etc.). I need to modify the DNS resolver to avoid service discovery in a k8s cluster.
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.
I was just thinking that it'd be better to route transport
through the proxy.dial
, the current approach when the transport is independent looks pretty strange.
It will also effectively solve this issue: #24 without any config changes.
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.
Merge Request to allow the possibility of specifying your own timeout, transport and dialer.