Taragolis commented on code in PR #28706:
URL: https://github.com/apache/airflow/pull/28706#discussion_r1063300999
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -57,8 +57,14 @@ def wrapper(*args, **kwargs) -> T:
if "bucket_name" not in bound_args.arguments:
self = args[0]
- if self.conn_config and self.conn_config.schema:
- bound_args.arguments["bucket_name"] = self.conn_config.schema
+ if self.aws_conn_id:
+ conn_args = self.get_connection(self.aws_conn_id).extra_dejson
Review Comment:
Get connection it is quite expensive call there is no reason call it every
time when decorated method called.
IMHO better:
1. create new property/attribute in
[AwsConnectionWrapper](https://github.com/apache/airflow/blob/8290ade26deba02ca6cf3d8254981b31cf89ee5b/airflow/providers/amazon/aws/utils/connection_wrapper.py#L76)
which store this dictionary
2. Add method to `get_service_config(service_name: str)` to
AwsConnectionWrapper which return config for specific config service
```python
def get_service_config(service_name: str) -> dict:
return self.service_config.get(service_name, {})
```
3. Add property into
[AwsGenericHook](https://github.com/apache/airflow/blob/8290ade26deba02ca6cf3d8254981b31cf89ee5b/airflow/providers/amazon/aws/hooks/base_aws.py#L379)
which associated with selected `service_name`
```python
@property
def service_config() -> dict:
service_name = client_type or resource_type
return self.conn_config.get_service_config(service_name)
```
In this case we only call get_connection once because
`AwsGenericHook.conn_config` is cached property.
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -57,8 +57,14 @@ def wrapper(*args, **kwargs) -> T:
if "bucket_name" not in bound_args.arguments:
self = args[0]
- if self.conn_config and self.conn_config.schema:
- bound_args.arguments["bucket_name"] = self.conn_config.schema
+ if self.aws_conn_id:
+ conn_args = self.get_connection(self.aws_conn_id).extra_dejson
Review Comment:
This also make integration with other services easier
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -57,8 +57,14 @@ def wrapper(*args, **kwargs) -> T:
if "bucket_name" not in bound_args.arguments:
self = args[0]
- if self.conn_config and self.conn_config.schema:
- bound_args.arguments["bucket_name"] = self.conn_config.schema
+ if self.aws_conn_id:
+ conn_args = self.get_connection(self.aws_conn_id).extra_dejson
Review Comment:
And you need only call in decorator something like this
```python
if "bucket_name" in self.service_config:
...
```
##########
tests/providers/amazon/aws/hooks/test_s3.py:
##########
@@ -466,16 +466,26 @@ def test_delete_bucket_if_not_bucket_exist(self,
s3_bucket):
assert mock_hook.delete_bucket(bucket_name=s3_bucket,
force_delete=True)
assert ctx.value.response["Error"]["Code"] == "NoSuchBucket"
- @mock.patch.object(S3Hook, "get_connection",
return_value=Connection(schema="test_bucket"))
+ from airflow.providers.amazon.aws.utils.connection_wrapper import
AwsConnectionWrapper
Review Comment:
Import should be defined in top level, unless we have strong reason import
here
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -57,8 +57,14 @@ def wrapper(*args, **kwargs) -> T:
if "bucket_name" not in bound_args.arguments:
self = args[0]
- if self.conn_config and self.conn_config.schema:
- bound_args.arguments["bucket_name"] = self.conn_config.schema
Review Comment:
We need also provide backward compatibility with current version.
Basically we need to check `bucket_name` in both places:
1. In `service_config.s3.bucket_name`
2. Fallback to `conn_config. schema` and depreciation warning
This also could be done in
[AwsConnectionWrapper](https://github.com/apache/airflow/blob/8290ade26deba02ca6cf3d8254981b31cf89ee5b/airflow/providers/amazon/aws/utils/connection_wrapper.py#L76)
--
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]
