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

Remove usage of http.DefaultClient #3532

Merged
merged 3 commits into from
Oct 20, 2015
Merged

Remove usage of http.DefaultClient #3532

merged 3 commits into from
Oct 20, 2015

Conversation

jefferai
Copy link
Member

No description provided.

@apparentlymart
Copy link
Contributor

Thanks for this... Though I am not sure I understand the motivation. Was this discussed somewhere else?

@jefferai
Copy link
Member Author

@apparentlymart Yes, it spun off of an internal discussion. I'm on a crusade to remove usage of http.DefaultClient as it very easily causes strange and hard-to-diagnose problems like hashicorp/vault#700. As http.DefaultClient is literally just a &http.Client{}, except it shares state implicitly instead of explicitly, I'm removing it across our projects.

The next step in Terraform will be going through dependencies and ensuring that, where possible, code paths in dependencies that use DefaultClient are instead given their own clients. I'll submit a PR for that later today.

Tests should certainly be run to ensure that nothing was relying on this behavior, but in that case the right fix isn't to use DefaultClient, it's to pass in the properly-configured client to the library for usage.

@jefferai
Copy link
Member Author

Hold off on merging this for the moment; there is an upstream fix going into packngo, and once it does I'll update this.

@brockoffdev
Copy link

👍 This worked well as a fix for hashicorp/vault#700

@jefferai
Copy link
Member Author

Done -- ready to merge!

phinze added a commit that referenced this pull request Oct 20, 2015
@phinze phinze merged commit 15a36d0 into master Oct 20, 2015
@phinze phinze deleted the remove-default-client branch October 20, 2015 15:42
@ghost
Copy link

ghost commented Apr 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants