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

Proxy environment variables are only used when they are non-empty #674

Merged
merged 9 commits into from
Feb 18, 2021

Conversation

racinmat
Copy link
Contributor

@racinmat racinmat commented Feb 16, 2021

Motivation:
Once environment variable is set, it's quite hard to unset it, e.g. in docker it's impossible to unset environment variable once it's set, see: https://stackoverflow.com/questions/55789409/how-to-unset-env-in-dockerfile

Lots of other software treat empty environment variable in same way as unset environment variable.
For example here https://docs.pivotal.io/application-service/2-8/operating/config-proxy.html in troubleshooting section.
Also curl states in https://curl.se/libcurl/c/CURLOPT_PROXY.html
"Setting the proxy string to "" (an empty string) will explicitly disable the use of a proxy, even if there is an environment variable set for it."

Git mentions them too in https://git-scm.com/docs/git-config/2.9.5#Documentation/git-config.txt-httpproxy , linking curl documentation, so I assume the behavior is same as in curl.

I'm not sure how exactly it's in Pkg, but for correct functionality of Pkg.jl behind proxy setting them is advised too, e.g. here: https://discourse.julialang.org/t/install-packages-behind-the-proxy/23298/18 but I'm not sure if Pkg.jl handles them on its own or delegates it to git, which delegates it to curl.

Fixes #671 .

I'm not sure if this is desired behavior or not, so let's discuss this if we want to have such behavior or if it's better to keep the old one.

I wanted to also add tests for this, but I couldn't find a way to call some method with environment variable set without affecting it globally.

Or should I simply add tests with

ENV["HTTP_PROXY"] = ""
@test getproxy.....
delete!(ENV, "HTTP_PROXY")

?

@quinnj
Copy link
Member

quinnj commented Feb 16, 2021

There's the withenv function:

help?> withenv
search: withenv

  withenv(f::Function, kv::Pair...)

  Execute f in an environment that is temporarily modified (not replaced as in setenv) by zero or more "var"=>val arguments kv. withenv is
  generally used via the withenv(kv...) do ... end syntax. A value of nothing can be used to temporarily unset an environment variable (if it
  is set). When withenv returns, the original environment has been restored.

Copy link
Member

@christopher-dG christopher-dG left a comment

Choose a reason for hiding this comment

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

This seems like a fine idea to me, although I'm not super familiar with how proxies work.

src/ConnectionRequest.jl Outdated Show resolved Hide resolved
Co-authored-by: Chris de Graaf <me@cdg.dev>
@codecov-io
Copy link

codecov-io commented Feb 16, 2021

Codecov Report

Merging #674 (9df3dfe) into master (a469ac6) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   77.49%   77.59%   +0.09%     
==========================================
  Files          36       36              
  Lines        2333     2334       +1     
==========================================
+ Hits         1808     1811       +3     
+ Misses        525      523       -2     
Impacted Files Coverage Δ
src/ConnectionRequest.jl 53.12% <100.00%> (+4.68%) ⬆️
src/Servers.jl 65.60% <0.00%> (-1.28%) ⬇️
src/Strings.jl 100.00% <0.00%> (ø)
src/IOExtras.jl 72.41% <0.00%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a469ac6...9df3dfe. Read the comment docs.

more syntax sugar
@racinmat
Copy link
Contributor Author

Added tests, I hope it's a right place, I can move them somewhere else if it's needed.

racinmat and others added 2 commits February 17, 2021 08:10
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
@racinmat
Copy link
Contributor Author

Something more, or is it ready for merge? Tests are failing on nightly, but they seem totally unrelated to my PR.

@racinmat
Copy link
Contributor Author

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

LGTM (but I don't have the power to take action).

Might be good to reset the env since tests will fail otherwise for users that do have them set (see comments).

test/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
racinmat and others added 2 commits February 18, 2021 16:54
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
@christopher-dG christopher-dG merged commit c0b9b5c into JuliaWeb:master Feb 18, 2021
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.

empty HTTP_PROXY and HTTPS_PROXY are still used
5 participants