Skip to content

feat: add support for https_proxy environment variable (#415)#650

Open
recreators01 wants to merge 2 commits into
Open-Wine-Components:mainfrom
recreators01:https_proxy
Open

feat: add support for https_proxy environment variable (#415)#650
recreators01 wants to merge 2 commits into
Open-Wine-Components:mainfrom
recreators01:https_proxy

Conversation

@recreators01
Copy link
Copy Markdown

https_proxy should be enough.
Regarding the changes to umu_runtime.py, the URL must include a scheme when using ProxyManager; otherwise, it will not work.

@loathingKernel
Copy link
Copy Markdown
Contributor

Please rebase your PR properly on top of current main branch to avoid merge artifacts. Also avoid unnecessary style changes in unrelated code.

@recreators01
Copy link
Copy Markdown
Author

Please rebase your PR properly on top of current main branch to avoid merge artifacts. Also avoid unnecessary style changes in unrelated code.

ok

@recreators01
Copy link
Copy Markdown
Author

Close:#415
This also resolves the issue that URLs without a scheme (i.e., 'https://') are deprecated and will raise an error in urllib3 v3.0.

Comment thread umu/umu_run.py
http_pool: PoolManager
if "https_proxy" in os.environ:
http_pool = ProxyManager(
proxy_url=os.environ["https_proxy"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is lowercase https_proxy a common env variable or something you came up with for this PR? If it isn't common, IMO it would better to be uppercase, and specific to UMU, i.e. UMU_HTTP_PROXY to match the existing UMU_HTTP_RETRIES and UMU_HTTP_TIMEOUT

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Common env variable. Most app use it, e.g. curl wget git python urllib/urllib2.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, for future reference this is the rationale curl provides: https://everything.curl.dev/usingcurl/proxies/env.html#http_proxy-in-lower-case-only

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.

2 participants