Taragolis commented on code in PR #29452:
URL: https://github.com/apache/airflow/pull/29452#discussion_r1108381943
##########
airflow/providers/amazon/aws/transfers/dynamodb_to_s3.py:
##########
@@ -103,12 +114,14 @@ def __init__(
self,
*,
dynamodb_table_name: str,
+ source_aws_conn_id: str = "aws_default",
s3_bucket_name: str,
file_size: int,
dynamodb_scan_kwargs: dict[str, Any] | None = None,
s3_key_prefix: str = "",
process_func: Callable[[dict[str, Any]], bytes] =
_convert_item_to_json_bytes,
- aws_conn_id: str = "aws_default",
+ dest_aws_conn_id: ArgNotSet | str = NOTSET,
+ aws_conn_id: ArgNotSet | str = NOTSET,
Review Comment:
```suggestion
dest_aws_conn_id: str | None | ArgNotSet = NOTSET,
aws_conn_id: str | None | ArgNotSet = NOTSET,
```
##########
airflow/providers/amazon/aws/transfers/dynamodb_to_s3.py:
##########
@@ -118,10 +131,18 @@ def __init__(
self.dynamodb_scan_kwargs = dynamodb_scan_kwargs
self.s3_bucket_name = s3_bucket_name
self.s3_key_prefix = s3_key_prefix
- self.aws_conn_id = aws_conn_id
+ if aws_conn_id is not NOTSET:
+ warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=3)
+ self.source_aws_conn_id = aws_conn_id
+ else:
+ self.source_aws_conn_id = source_aws_conn_id
+ if dest_aws_conn_id is NOTSET:
+ self.dest_aws_conn_id = self.source_aws_conn_id
+ else:
+ self.dest_aws_conn_id = dest_aws_conn_id
Review Comment:
"Tests Police" here 🤣 , we need also test this one. Just sure that we cover
all cases with different inputs of `aws_conn_id`, `source_aws_conn_id`,
`dest_aws_conn_id`, especially (but not limited) cases which is possible for
current version of provider:
1. User not provide anything, for example user expected to use `aws_default`
connection 🙄
2. User provide value only to `aws_conn_id`
Might be something like this:
https://github.com/apache/airflow/blob/16fddbae83d03c9b3e2d249cc8852fb006c65c3b/tests/providers/slack/hooks/test_slack_webhook.py#L194-L211
##########
airflow/providers/amazon/aws/transfers/dynamodb_to_s3.py:
##########
@@ -103,12 +114,14 @@ def __init__(
self,
*,
dynamodb_table_name: str,
+ source_aws_conn_id: str = "aws_default",
Review Comment:
```suggestion
source_aws_conn_id: str | None = "aws_default",
```
##########
airflow/providers/amazon/aws/transfers/dynamodb_to_s3.py:
##########
@@ -135,7 +156,7 @@ def execute(self, context: Context) -> None:
raise e
finally:
if err is None:
- _upload_file_to_s3(f, self.s3_bucket_name,
self.s3_key_prefix, self.aws_conn_id)
+ _upload_file_to_s3(f, self.s3_bucket_name,
self.s3_key_prefix, str(self.dest_aws_conn_id))
Review Comment:
Why we need string representation of `self.dest_aws_conn_id` 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]
