seanmuth commented on code in PR #68305:
URL: https://github.com/apache/airflow/pull/68305#discussion_r3546750638
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -576,7 +583,14 @@ def test_jwt_auth_type(self, mock_hvac):
"renewable": False,
"lease_duration": 0,
"data": {
- "data": {"conn_uri":
"postgresql://airflow:airflow@host:5432/airflow"},
+ "data": {
+ "conn_type": "postgres",
+ "login": "airflow",
+ "password": "airflow",
+ "host": "host",
+ "port": "5432",
+ "schema": "airflow",
+ },
Review Comment:
The URI-with-JWT path is still covered — `test_jwt_auth_type_conn_uri`
(added in `c948bf7a29`, just above the field-based test) sets
`auth_type="jwt"`, returns a `conn_uri` secret, and asserts both that the
verbatim URI is returned and that `jwt_login` was called.
`test_jwt_auth_type_field_based` is the complementary case for field-based
secrets, so both the URI and dict paths are exercised under JWT auth. Happy to
consolidate or rename them if you'd prefer.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @seanmuth before posting
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -82,7 +82,9 @@ def variable_result(self):
}
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
- def test_get_connection(self, mock_hvac):
+ def test_get_conn_value(self, mock_hvac):
+ import json
Review Comment:
Done in `59b4285e77` — hoisted a single `import json` to the module top and
dropped the five in-body copies.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @seanmuth before posting
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -224,32 +224,30 @@ def _get_team_or_global_secret(self, base_path: str |
None, team_name: str | Non
return self._get_secret_with_base(path, key)
- # Make sure connection is imported this way for type checking, otherwise
when importing
- # the backend it will get a circular dependency and fail
- if TYPE_CHECKING:
- from airflow.models.connection import Connection
-
- def get_connection(self, conn_id: str, team_name: str | None = None) ->
Connection | None:
+ def get_conn_value(self, conn_id: str, team_name: str | None = None) ->
str | None:
"""
- Get connection from Vault as secret.
+ Retrieve a connection from Vault as a serialized string.
- Prioritize conn_uri if exists, if not fall back to normal Connection
creation.
+ Returns the ``conn_uri`` value verbatim when present, otherwise
serializes
+ the secret dict to JSON. The base-class ``get_connection``
deserializes the
+ returned string using the Connection class that the framework injected
for the
+ current execution context (ORM Connection on the server, SDK
Connection in
+ workers), which avoids triggering SQLAlchemy mapper initialization in
Review Comment:
Done in `59b4285e77` — qualified the docstring to "On Airflow 3.2+…" and
spelled out that on the older supported releases (2.11 / 3.0 / 3.1) the base
`deserialize_connection` imports the ORM `Connection` directly, so the
mapper-initialization avoidance doesn't apply there.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @seanmuth before posting
--
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]
