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

fix(auth): handle non-Transport DefaultTransport #10733

Merged

Conversation

adapap
Copy link
Contributor

@adapap adapap commented Aug 22, 2024

If a client does not specify opts.BaseRoundTripper in httptransport.NewClient, avoid a panic if the global http.DefaultTransport has been overwritten to something that is not an instance of *http.Transport.

Our use case is to create a new Google API client, but the constructor for NewClient does not allow passing a BaseRoundTripper option as seen here.

Fixes #10742.

@adapap adapap requested a review from a team as a code owner August 22, 2024 01:26
@adapap adapap force-pushed the adapap/non-default-transport-newclient branch from ea803b5 to 7fd4e3a Compare August 22, 2024 01:38
@codyoss
Copy link
Member

codyoss commented Aug 22, 2024

@adapap Can you please create an issue for discussing the change, thanks.

@codyoss codyoss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 22, 2024
@adapap adapap force-pushed the adapap/non-default-transport-newclient branch from 7fd4e3a to be8c35f Compare August 22, 2024 18:27
@adapap
Copy link
Contributor Author

adapap commented Aug 22, 2024

@codyoss Based on our discussion in #10742, I've updated this PR to export BaseTransport from the auth/internal/transport package, and use this as the fallback instead of accepting the http.DefaultTransport if it was modified.

@codyoss codyoss removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 22, 2024
If a client does not specify opts.BaseRoundTripper in httptransport.NewClient, avoid a panic if the global http.DefaultTransport has been overwritten to something that is not an instance of *http.Transport.
@adapap adapap force-pushed the adapap/non-default-transport-newclient branch from f985ea5 to e034124 Compare August 22, 2024 20:51
@codyoss codyoss added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 23, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2024
@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2024
@codyoss codyoss removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 23, 2024
@codyoss codyoss merged commit 98d91dc into googleapis:main Aug 23, 2024
8 checks passed
@codyoss
Copy link
Member

codyoss commented Aug 23, 2024

@adapap Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth: panic when calling httptransport.NewClient if http.DefaultTransport was overwritten
3 participants