http: Add the ability to not follow redirects #278
Conversation
Currently, when we unmarshal, we get the default value, which is to follow redirects. Signed-off-by: Julien Pivotto <[email protected]>
Signed-off-by: Julien Pivotto <[email protected]>
config/http_config.go
Outdated
| @@ -111,6 +111,10 @@ type HTTPClientConfig struct { | |||
| ProxyURL URL `yaml:"proxy_url,omitempty"` | |||
| // TLSConfig to use to connect to the targets. | |||
| TLSConfig TLSConfig `yaml:"tls_config,omitempty"` | |||
| // FollowRedirects specifies wheter the client should follow the HTTP 3xx redirects. | |||
There was a problem hiding this comment.
wheter → whether
And I'd prefer "follow HTTP 3xx redirects" over "follow the HTTP 3xx rediracts".
config/http_config.go
Outdated
| @@ -172,6 +176,9 @@ func (c *HTTPClientConfig) Validate() error { | |||
| // UnmarshalYAML implements the yaml.Unmarshaler interface | |||
| func (c *HTTPClientConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| type plain HTTPClientConfig | |||
| *c = HTTPClientConfig{ | |||
There was a problem hiding this comment.
This overrides anything that might be pre-populated in *c. Not sure if we ever do that, but wouldn't it be safer (and easier) to do c.FollowRedirects = true?
There was a problem hiding this comment.
No because C is nil at this point.
We do this in Prometheus:
There was a problem hiding this comment.
I have switched to the exact same pattern as prometheus (expose default struct)
There was a problem hiding this comment.
Just FTR: If c is nil, then L179, as above, panics.
There was a problem hiding this comment.
mmmmh yes indeed. but still, this is the pattern we use everywhere, let's stick to it?
Signed-off-by: Julien Pivotto <[email protected]>
roidelapluie
force-pushed
the
redirect
branch
from
3f9c8cc
to
f9eb271
Compare
config/http_config.go
Outdated
| @@ -111,6 +116,10 @@ type HTTPClientConfig struct { | |||
| ProxyURL URL `yaml:"proxy_url,omitempty"` | |||
| // TLSConfig to use to connect to the targets. | |||
| TLSConfig TLSConfig `yaml:"tls_config,omitempty"` | |||
| // FollowRedirects specifies wheter the client should follow HTTP 3xx redirects. | |||
config/http_config_test.go
Outdated
| @@ -317,7 +357,7 @@ func TestNewClientFromConfig(t *testing.T) { | |||
| } | |||
| response, err := client.Get(testServer.URL) | |||
| if err != nil { | |||
| t.Errorf("Can't connect to the test server using this config: %+v", validConfig.clientConfig) | |||
| t.Errorf("Can't connect to the test server using this config: %+v %v", validConfig.clientConfig, err) | |||
There was a problem hiding this comment.
It's kind of hard to see that there is an error following after the config. How about:
"Can't connect to the test server using config %+v: %v"
| @@ -172,6 +181,7 @@ func (c *HTTPClientConfig) Validate() error { | |||
| // UnmarshalYAML implements the yaml.Unmarshaler interface | |||
| func (c *HTTPClientConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| type plain HTTPClientConfig | |||
| *c = DefaultHTTPClientConfig | |||
There was a problem hiding this comment.
As said, this wouldn't work if c were nil. (Which implies that c is not nil rather than that we are doing anything wrong here.)
Signed-off-by: Julien Pivotto <[email protected]>
|
Thanks for your review, I think this is good to go. |
|
Yes, sure, that's why I had approved it already. (o: |
Currently, when we unmarshal, we get the default value, which is to
follow redirects.
Related to prometheus/prometheus#5108
Signed-off-by: Julien Pivotto [email protected]