Add http proxy support for Omnibus::NetFetcher #77
Conversation
|
This looks off to a good start. We're following the same contribution process for this project as for Chef. Would you be willing to sign a CLA so we can merge your code? Thank you for taking the time to help make Chef better! We require a Contributor License Agreement (CLA) from contributors to help protect all Chef users. It is easy to complete one online. Instructions to do so are on our wiki, which is linked below. Once your account is updated, you will have access to 'resolve' tickets that indicates to the code review team that your contribution is ready for review. http://wiki.opscode.com/display/chef/How+to+Contribute |
| @@ -88,7 +88,14 @@ def get_with_redirect(url, headers, limit = 10) | |||
| end | |||
|
|
|||
| req = Net::HTTP::Get.new(url.request_uri, headers) | |||
| http_client = Net::HTTP.new(url.host, url.port) | |||
|
|
|||
| no_proxy = (ENV['NO_PROXY'] || ENV['no_proxy'] || "").split(/\s*,\s*/) | |||
There was a problem hiding this comment.
A small helper function for retrieving ENV vars in a case insensitive way might help clean this up. Something along the lines of:
get_env(name)
ENV[name.downcase] || ENV[name.upcase] || nil
end
There was a problem hiding this comment.
This bit would also benefit from a brief comment explaining how the proxy stuff is intended to work.
|
I signed a CLA from Opscode some time ago already . My Opscode JIRA username is databus23. I added another commit where I tried to clean up the code according to your feedback. |
| @@ -104,6 +109,31 @@ def get_with_redirect(url, headers, limit = 10) | |||
| end | |||
| end | |||
|
|
|||
| #search environment variable as given, all lowercase and all upper case | |||
There was a problem hiding this comment.
What's the benefit of #get_env? It looks like you're passing environment variables in uppercase, looking for them, looking for them again downcased, and then again upcased. I've never used environment variables that were anything other than uppercase. I think we're risking some confusion if someone sets a variable with different case by accident.
There was a problem hiding this comment.
The proxy related environment variables are not required to by all uppercase in my experience. At least all the common cli tools (wget, git, curl) support proxy related variables in lowercase.
The systems I'm working with have proxy environment variables names in lowercase and it never caused any problems.
So the intended benefit of get_env is to read environment variables regardless if the are written uppercase or lowercase. Looking them up as is (ENV[name]) is not strictly necessary, I just wanted the method to be able to read variables in mixed case provided you pass in the exact string.
There was a problem hiding this comment.
Matching behavior of curl is a good argument. I don't think this is overly complicated or confusing.
|
Thanks again for the patch. I've added a bit of test coverage and merged this. |
This is a simple stab add adding http proxy support when fetching tarballs