-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Change httpx
to requests
in file_task_handler
#39799
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
httpx does not support CIDRs in NO_PROXY
I wonder if it's possible to implement a test for this support since it's the main motivation for this change.
httpx
to requests
in file_task_handler
I guess the main prom that there is no standardisation for Most of the libraries use stdlib implementation from Lib/urllib/request.py Request use own implementation which supports CIDR blocks, but only for ipv4: import os
os.environ["no_proxy"] = "localhost,127.0.0.1,10.0.0.0/8,172.16.1.*,1:2:3::/64"
os.environ["NO_PROXY"] = "localhost,127.0.0.1,192.168.1.0/16"
# stdlib implementation
from urllib.request import proxy_bypass_environment
# requests implementation
from requests.sessions import should_bypass_proxies
hosts = ("localhost", "127.0.0.1", "127.0.0.2", "10.0.0.0", "192.168.1.32", "172.16.1.2", "[1:2:3::4]")
for host in hosts:
print(f" {host} ".center(80, '='))
print(f"stdlib Proxy Bypass Environment: {proxy_bypass_environment(host, None)}")
print(f"requests Proxy Bypass Environment: {should_bypass_proxies(f"http://{host}", None)}") ================================== localhost ===================================
stdlib Proxy Bypass Environment: True
requests Proxy Bypass Environment: True
================================== 127.0.0.1 ===================================
stdlib Proxy Bypass Environment: True
requests Proxy Bypass Environment: True
================================== 127.0.0.2 ===================================
stdlib Proxy Bypass Environment: False
requests Proxy Bypass Environment: False
=================================== 10.0.0.0 ===================================
stdlib Proxy Bypass Environment: False
requests Proxy Bypass Environment: True
================================= 192.168.1.32 =================================
stdlib Proxy Bypass Environment: False
requests Proxy Bypass Environment: False
================================== 172.16.1.2 ==================================
stdlib Proxy Bypass Environment: False
requests Proxy Bypass Environment: False
================================== [1:2:3::4] ==================================
stdlib Proxy Bypass Environment: False
requests Proxy Bypass Environment: False I don't know what is supposed to be happen in case if we change library in the future, e.g. from So probably it is fine for replace |
@hussein-awala @Taragolis |
I would not spend a time for writing a test proxy. If there is any changes in the future then this test should be adopted by someone who will made this changes. About new library, it just a potential scenario but maybe it is never happen. |
de18491
to
f48b958
Compare
f48b958
to
b771985
Compare
- httpx does not support CIDRs in NO_PROXY - simply, convert httpx to requests, issues done - related issue: apache#39794
b771985
to
c47b3f1
Compare
I believe, this is the last httpx use in Airflow core - httpx is used in a few other scripts and API client (and it could be easily replaced by requests as well), so we could get rid of httpx dependency in |
On the other hand with -> #40256 it turned out that we have some implicit dependencies on httpx in core, so removing it is not good idea. |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* Change httpx to requests in file_task_handler - httpx does not support CIDRs in NO_PROXY - simply, convert httpx to requests, issues done - related issue: #39794 * Add cidr no_proxy test test_log_handlers.py * Apply monkeypatch fixture --------- Co-authored-by: scott-py <[email protected]> (cherry picked from commit 1ddadf5)
* Change httpx to requests in file_task_handler - httpx does not support CIDRs in NO_PROXY - simply, convert httpx to requests, issues done - related issue: apache#39794 * Add cidr no_proxy test test_log_handlers.py * Apply monkeypatch fixture --------- Co-authored-by: scott-py <[email protected]>
closes: #39794
httpx
torequests
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.