ashb commented on code in PR #73116:
URL: https://github.com/apache/airflow/pull/73116#discussion_r4193632375
##########
devel-common/src/tests_common/test_utils/in_process_taskrun.py:
##########
@@ -55,27 +59,42 @@
_XCOM_PATH_PARTS = 5 # /xcoms/{dag_id}/{run_id}/{task_id}/{key}
+def _resolve_sdk_httpx() -> ModuleType:
+ """Return the httpx package the *installed* Task SDK ``Client`` subclasses.
+
+ Compat jobs pair this helper with a released SDK still on ``httpx``. A
transport from the
Review Comment:
```suggestion
Needed as these test_utils run against multiple versions of Airflow/sdk.
This can be
removed when the oldest version we test against is Airflow 3.4.
Compat jobs pair this helper with a released SDK still on ``httpx``. A
transport from the
```
##########
task-sdk/.pre-commit-config.yaml:
##########
@@ -65,3 +65,12 @@ repos:
pass_filenames: false
files: ^.*\.py$
require_serial: true
+ - repo: https://github.com/boidolr/ast-grep-pre-commit
+ rev: 7bae9c67e1a003cd5ef629c64f0466dc46556b3b # frozen: 0.45.3
+ hooks:
+ - id: ast-grep
Review Comment:
I don't think we need to introduce ast-grep and depend on a new
providers/module for this. Ruff has banned imports I think.
Please see if you can do it via task-sdk/pyproject.toml?
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -1941,29 +1936,53 @@ def handle_request(request: httpx.Request) ->
httpx.Response:
assert result.responded_at == timezone.datetime(2025, 7, 3, 0, 0, 0)
-class TestSSLContextCaching:
+class TestSSLContext:
Review Comment:
Needless rename?
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -20,15 +20,19 @@
import json
import pickle
import sys
-from datetime import datetime, timezone as dt_timezone
+from datetime import datetime, timedelta, timezone as dt_timezone
from typing import TYPE_CHECKING
from unittest import mock
-import certifi
-import httpx
+import httpx2
import pytest
import time_machine
+import truststore
import uuid6
+from cryptography import x509
+from cryptography.hazmat.primitives import hashes, serialization
+from cryptography.hazmat.primitives.asymmetric import ec
+from cryptography.x509.oid import NameOID
Review Comment:
These are scary imports :) Can we scope these to just the one `ca_file` that
needs them? It would make me feel happier that the "unsafe" bit is limited to
one area.
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -1941,29 +1936,53 @@ def handle_request(request: httpx.Request) ->
httpx.Response:
assert result.responded_at == timezone.datetime(2025, 7, 3, 0, 0, 0)
-class TestSSLContextCaching:
+class TestSSLContext:
@pytest.fixture(autouse=True)
def clear_ssl_context_cache(self):
Client._get_ssl_context_cached.cache_clear()
yield
Client._get_ssl_context_cached.cache_clear()
- def test_cache_hit_on_same_parameters(self):
- ca_file = certifi.where()
+ @pytest.fixture
+ def ca_file(self, tmp_path):
+ key = ec.generate_private_key(ec.SECP256R1())
+ name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "test-ca")])
+ now = datetime.now(dt_timezone.utc)
+ cert = (
+ x509.CertificateBuilder()
+ .subject_name(name)
+ .issuer_name(name)
+ .public_key(key.public_key())
+ .serial_number(x509.random_serial_number())
+ .not_valid_before(now)
+ .not_valid_after(now + timedelta(days=1))
+ .add_extension(x509.BasicConstraints(ca=True, path_length=None),
critical=True)
+ .sign(key, hashes.SHA256())
+ )
+ path = tmp_path / "ca.pem"
+ path.write_bytes(cert.public_bytes(serialization.Encoding.PEM))
+ return str(path)
+
+ @mock.patch.object(truststore.SSLContext, "load_verify_locations",
autospec=True)
Review Comment:
I'm slightly nervous about mocking this.
Also: if we are mocking this, do we actually need a real CA file on disk? If
feels like it should be one or the other, but not both?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
