Make request timeout and TLS properties configurable.#122
Conversation
|
Hi @oppegard and @LukeWinikates, this PR is an extension of #111 with a cleaner git history. I explained the decision to create this PR in this comment in #111. Thank you for helping me through this as I learn. |
|
Hello @keep94, nice to meet you! Thank you for taking the time to review -- I will start addressing these today. |
5f4afcd to
d091daf
Compare
Add new configuration options to README. Signed-off-by: Devon Warshaw <warshawd@vmware.com> Signed-off-by: Jeanette Booher <jbooher@vmware.com> Signed-off-by: Supraja Narasimhan <suprajan@vmware.com>
d091daf to
1609951
Compare
…ional arguments. Signed-off-by: Devon Warshaw <warshawd@vmware.com>
Signed-off-by: Devon Warshaw <warshawd@vmware.com>
I made the changes and left the comments open so you can decide if they can be resolved. Thank you! |
keep94
left a comment
There was a problem hiding this comment.
Thanks for the changes and the great work. I had one more comment. Have to go, so I'll look over it again on Monday.
Signed-off-by: Supraja Narasimhan <suprajanarasimhan@vmware.com>
4590e5c to
fc91280
Compare
internal/reporter.go
Outdated
| func NewClient(timeout time.Duration, tlsConfig *tls.Config) *http.Client { | ||
| var client *http.Client | ||
| if tlsConfig == nil { | ||
| client = &http.Client{Timeout: timeout} |
There was a problem hiding this comment.
You could simplify this function like so:
if tlsConfig == nil {
return &http.Client{Timeout: timeout}
}
transport := &http.Transport{TLSClientConfig: tlsConfig}
return &http.Client{Timeout: timeout, Transport: transport}
There was a problem hiding this comment.
I'm confused by the change as-is @suprajanarasimhan:
if tlsConfig == nil {
client = &http.Client{Timeout: timeout}
}
transport := &http.Transport{TLSClientConfig: tlsConfig}
client = &http.Client{Timeout: timeout, Transport: transport}
return clientThe if condition will have no effect on client since it's overwritten below that. Did you mean to follow Travis' original suggestion?
if tlsConfig == nil {
return &http.Client{Timeout: timeout}
}
transport := &http.Transport{TLSClientConfig: tlsConfig}
return &http.Client{Timeout: timeout, Transport: transport}There was a problem hiding this comment.
@suprajanarasimhan, can we add test coverage with a nil and non-nil tlsConfig, to catch this logic error?
There was a problem hiding this comment.
Yes, oppegard is right. When tlsConfig == nil you want to return &http.Client{Timeout: timeout}
There was a problem hiding this comment.
As oppegard already stated, consider adding a test for internal.NewClient(). When the tlsConfig parameter is nil, the Transport field of returned http client should be nil.
There was a problem hiding this comment.
Hi @oppegard, thank for catching this logic error! 🙏🏽 @ktollivervmw, @jessepye and I added the test you and @keep94 recommended. Grateful for both of your thorough and informative reviews.
|
Hello @keep94! Coming back to this after the holidays and I made the changes you requested. Overall, your feedback exposed me to concepts and code style that I can try to apply in the future, too! I really appreciate it. 🙏🏽 |
|
I will have time Friday afternoon to take a look. Is that ok? |
|
I will revisit this feedback later today -- thank you! |
Signed-off-by: Kyle Tolliver <ktolliver@vmware.com>
Related to #6.
This is an extension of PR #111, which I closed due to having introduced undesired merge commits in git history.
Detailed description:
Make request timeout and TLS properties configurable.
Add new configuration options to README.
Signed-off-by: Devon Warshaw warshawd@vmware.com
Signed-off-by: Jeanette Booher jbooher@vmware.com
Signed-off-by: Supraja Narasimhan suprajan@vmware.com