client: some cleanups#46133
Merged
cpuguy83 merged 3 commits intomoby:masterfrom Aug 3, 2023
Merged
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
thaJeztah
commented
Aug 1, 2023
Comment on lines
+219
to
+222
| // tlsConfig returns the TLS configuration from the client's transport. | ||
| // It returns nil if the transport is not a [http.Transport], or if no | ||
| // TLSClientConfig is set. | ||
| func (cli *Client) tlsConfig() *tls.Config { |
Member
Author
There was a problem hiding this comment.
Small note; I put this function after defaultHTTPClient() so that all methods on the Client are together, and to put it closer to where it's used 😅 (@cpuguy83's PR put it above defaultHTTPClient()
inline the closures, and update the GoDoc to use doc-links to the related buildkit function. Signed-off-by: Sebastiaan van Stijn <[email protected]>
fallbackDial was only used in a single place, and it was defined far away from where it's used, so let's inline it, so that it's clear at a glance what we're doing. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This makes it slightly clearer what it does, as "resolve" may give the impression it's doing more than just returning the TLS config configured for the client. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
client_cleanup
branch
from
39fc562 to
fced566
Compare
Member
Author
|
@cpuguy83 ptal 🤗 |
rumpl
approved these changes
Aug 3, 2023
cpuguy83
approved these changes
Aug 3, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had these lying around, but didn't push them yet.
I modified the last commit from what I originally had (originally I inlined the code), after noticing that #45652 also refactored the same code, but we decided to make a
cli.tlsConfig()method, so I already introduced that method, for it to be updated with the OTEL changes in #45652client/buildkit: ClientOpts: update docs to use doc-links, and inline
inline the closures, and update the GoDoc to use doc-links to the related
buildkit function.
client: Dialer: inline fallbackDial
fallbackDial was only used in a single place, and it was defined far away
from where it's used, so let's inline it, so that it's clear at a glance
what we're doing.
client: move resolveTLSConfig to a Client.tlsConfig()
This makes it slightly clearer what it does, as "resolve" may give the
impression it's doing more than just returning the TLS config configured
for the client.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)