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)
|
hussein-awala
left a comment
There was a problem hiding this comment.
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: FalseI 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 <scott.py@kakaocorp.com> (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 <scott.py@kakaocorp.com>
| logs.append(response.text) | ||
| except Exception as e: | ||
| from httpx import UnsupportedProtocol | ||
| from requests.exceptions import InvalidSchema |
There was a problem hiding this comment.
should be
from requests.exceptions import InvalidURLotherwise the fix on EmptyOperator's log msg doesn't work #35536
i'm going to open a new PR later if no one beats me to it
original fix in apache#35536 invalidated by apache#39799
closes: #39794
httpxtorequests^ 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.rstor{issue_number}.significant.rst, in newsfragments.