dstandish commented on code in PR #24737:
URL: https://github.com/apache/airflow/pull/24737#discussion_r916262411
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -220,6 +224,7 @@ def __init__(
radius_host=radius_host,
radius_secret=self.connection.password,
radius_port=radius_port,
+ **client_kwargs
Review Comment:
so if you are up to it you could make this a little nicer by handling the
case where user specifies something both in client kwargs (e.g. from conn
extra) and it's already specified by the hook -- e.g. if user puts
`azure_tenant_id` in conn.extra.
so you 'd do something like
```
client_kwargs.update(
**dict(
...,
password=self.connection.password,
key_id=self.connection.login,
secret_id=self.connection.password,
role_id=role_id,
kubernetes_role=kubernetes_role,
kubernetes_jwt_path=kubernetes_jwt_path,
gcp_key_path=gcp_key_path,
gcp_keyfile_dict=gcp_keyfile_dict,
gcp_scopes=gcp_scopes,
azure_tenant_id=azure_tenant_id,
azure_resource=azure_resource,
radius_host=radius_host,
radius_secret=self.connection.password,
radius_port=radius_port,
...
)
```
and if you you want to go the extra mile you can support passing something
like `key_id` through extra also (cus, really, that's more natural / intuitive
than putting it in `login`)
so then what you'd want to do in tests is pretty much just verify that your
order of precedence is working properly.
##########
airflow/providers/hashicorp/hooks/vault.py:
##########
@@ -135,6 +136,9 @@ def __init__(
except ValueError:
raise VaultError(f"The version is not an int: {conn_version}.
")
+ if not client_kwargs:
+ client_kwargs = self.connection.extra_dejson.get("client_kwargs")
Review Comment:
should ensure that client_kwargs is at least a {} or else it will break when
you try to spread it with `**`
--
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]
