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

Add support for HTTP proxy env vars. #18629

Merged
merged 2 commits into from
Nov 6, 2019
Merged

Add support for HTTP proxy env vars. #18629

merged 2 commits into from
Nov 6, 2019

Conversation

xocasdashdash
Copy link
Contributor

Added a minimal transport to the artifactory client so that it support proxy environment variables. This should fix this issue #9403 .

Upstream there's this issue opened lusis/go-artifactory#9 but I think there might need to be some refactor on terraform's side to support the latest client.

@a2ron
Copy link

a2ron commented Aug 8, 2018

👍

@xocasdashdash
Copy link
Contributor Author

Also I can confirm that this fix works and makes TF follow the http_proxy and https_proxy env vars.

@apparentlymart
Copy link
Contributor

Thanks for working on this @xocasdashdash!

This seems like a good change, that we can merge. However, it is unfortunately likely to conflict with ongoing work in our 0.12 major feature development branch, so I'd like to pause on merging for the moment until we are able to land that branch into master. I expect it'll be more straightforward to update this relatively-small commit in the case of conflicts (which we'd be happy to do when the time comes) than to handle another conflict in the more-complex refactoring we've been working on for 0.12.

Due to the scale of the work in progress it's likely take a while longer to get the development branch in a shape ready to merge. Once we have merged it we plan to take a second look over the pending PRs and see what other changes we are reasonable to include in the 0.12 release.

Thanks again, and sorry for the delay while we work through these other changes.

@xocasdashdash
Copy link
Contributor Author

@apparentlymart ok! I'll keep my eye out when you land the changes on master so I'll rebase those changes in. I do believe that it might make sense to try to respect the proxy vars whenever possible in all the providers.

@xocasdashdash
Copy link
Contributor Author

@apparentlymart Hi! I've updated with the latest release from origin/master. Do you think this change can make it into the 0.11.9 release?

@xocasdashdash
Copy link
Contributor Author

Hi @apparentlymart !
Can I get some feedback on this? Anything else I can do for this to be merged?

@jbardin
Copy link
Member

jbardin commented Mar 29, 2019

Hi @xocasdashdash,

Thanks for the PR. The addition of the proxy setting seems like a good idea, but your transport is missing a number of parameters. Rather than setting them here, you can fetch a new configured transport using: https://godoc.org/github.com/hashicorp/go-cleanhttp#DefaultPooledTransport

@xocasdashdash
Copy link
Contributor Author

I'll make the change right now!

@apparentlymart apparentlymart self-assigned this Nov 6, 2019
@apparentlymart
Copy link
Contributor

Hi @xocasdashdash!

Sorry for the delay here. This looks great to me and I'm going to merge it now.

@apparentlymart apparentlymart merged commit d2bc7c2 into hashicorp:master Nov 6, 2019
@xocasdashdash xocasdashdash deleted the feature/implement-http-proxy-for-artifactory branch November 6, 2019 22:25
@ghost
Copy link

ghost commented Mar 29, 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 Mar 29, 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