Skip to content
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

Prevent using DefaultTransport #409

Merged
merged 3 commits into from
May 30, 2021

Conversation

alanhamlett
Copy link
Member

This adds to #395 to make sure NewTransport is used instead of DefaultTransport.

@alanhamlett alanhamlett force-pushed the bugfix/prevent-using-default-http-client branch from e704f09 to e61fe7f Compare May 22, 2021 15:07
pkg/api/transport.go Outdated Show resolved Hide resolved
@alanhamlett alanhamlett force-pushed the bugfix/prevent-using-default-http-client branch from e61fe7f to aebd079 Compare May 22, 2021 20:16
@gandarez gandarez added the bug Something isn't working label May 22, 2021
@alanhamlett alanhamlett requested a review from gandarez May 23, 2021 04:03
Copy link
Contributor

@dron22 dron22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use NewClient, as it was adjusted in #409, the client should always have the custom transport set. Thus default transport will never be used in the option functions and the changes here should not have any effect.

@alanhamlett Am I missing something?

@alanhamlett
Copy link
Member Author

Yea, I saw the same thing but client.Transport is nullable and it doesn't hurt to be extra sure.

@alanhamlett alanhamlett requested a review from dron22 May 23, 2021 21:46
@dron22
Copy link
Contributor

dron22 commented May 23, 2021

If we worry, that transport is unintentionally set to nil, we could also worry that transport is unintentionally set to DefaultTransport somewhere. 🤔

But alright, if you prefer double security, then let's do it. We do not need to change the signature of NewTransport to achieve this though. The previous implementation was simpler and using it as-is will make the reasoning about the option functions easier.

@alanhamlett
Copy link
Member Author

The previous implementation was simpler

The one where I used a GetOrCreateTransport function?

@alanhamlett alanhamlett force-pushed the bugfix/prevent-using-default-http-client branch from aebd079 to 24126c4 Compare May 24, 2021 15:56
@dron22
Copy link
Contributor

dron22 commented May 24, 2021

I meant the version before the changes in this PR (NewTransport()), which had no input arguments, and simply returned the custom transport. The new name HTTPTransport is also good, I mainly care about the input argument(s).

Reason: Passing the http client as an input argument makes it harder to reason about the function. Instead of just looking at the function signature, you need to read the implementation to be sure about what it is doing. Also the option of passing nil is handled, which adds more complexity.

@gandarez
Copy link
Member

Is this PR still relevant?

@alanhamlett
Copy link
Member Author

Is this PR still relevant?

I think so... we want to remove any coding calling http.DefaultTransport.

Copy link
Contributor

@dron22 dron22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referring to my comment above here.

pkg/api/transport.go Show resolved Hide resolved
@alanhamlett alanhamlett force-pushed the bugfix/prevent-using-default-http-client branch from 24126c4 to 75cada3 Compare May 30, 2021 04:13
@alanhamlett alanhamlett requested a review from dron22 May 30, 2021 04:15
@alanhamlett alanhamlett merged commit 1a245fb into develop May 30, 2021
@alanhamlett alanhamlett deleted the bugfix/prevent-using-default-http-client branch May 30, 2021 04:49
This was referenced Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants