amoghrajesh commented on code in PR #68305:
URL: https://github.com/apache/airflow/pull/68305#discussion_r3489664334
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -224,32 +224,68 @@ 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):
Review Comment:
Type annotation please.
##########
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:
There's no coverage for `conn_uri` now?
The JWT auth test in particular was validating that JWT authenticated vault
reads with `conn_uri` worked end-to-end & switching it to field-based means
there is no test confirming URI secrets work with JWT auth
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -121,8 +122,10 @@ def test_get_connection(self, mock_hvac):
}
test_client = VaultBackend(**kwargs)
+ if hasattr(test_client, "_set_connection_class"):
+ test_client._set_connection_class(SdkConnection)
connection = test_client.get_connection(conn_id="test_postgres")
- assert connection.get_uri() ==
"postgresql://airflow:airflow@host:5432/airflow?foo=bar&baz=taz"
+ assert connection.get_uri() ==
"postgres://airflow:airflow@host:5432/airflow?foo=bar&baz=taz"
Review Comment:
Intentional?
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -192,7 +197,7 @@ def test_get_connection_value_multi_team(
for path in exp_paths
]
)
- assert connection.get_uri() ==
"postgresql://airflow:airflow@host:5432/airflow?foo=bar&baz=taz"
+ assert connection.get_uri() ==
"postgres://airflow:airflow@host:5432/airflow?foo=bar&baz=taz"
Review Comment:
Same here.
--
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]
