ferruzzi commented on code in PR #47321:
URL: https://github.com/apache/airflow/pull/47321#discussion_r1980579642
##########
providers/amazon/src/airflow/providers/amazon/aws/operators/s3.py:
##########
@@ -682,6 +693,7 @@ def execute(self, context: Context):
if self.transform_script is None and self.select_expression is None:
raise AirflowException("Either transform_script or
select_expression must be specified")
+ # Keep these hooks constructed here since we are using two unique
conn_ids
Review Comment:
Good comment :+1: In that case, the other changes you made to the parent
and setting the aws_hook_class in this particular operator aren't doing
anything, right? I guess it doesn't hurt to do it for the consistency.
##########
providers/amazon/tests/unit/amazon/aws/operators/test_s3.py:
##########
@@ -870,8 +868,7 @@ def test_validate_keys_and_prefix_in_execute(self, keys,
prefix, from_datetime,
assert objects_in_dest_bucket["Contents"][0]["Key"] == key_of_test
@pytest.mark.parametrize("keys", ("path/data.txt", ["path/data.txt"]))
Review Comment:
```suggestion
@pytest.mark.parametrize(
"keys",
[
pytest.param("path/data.txt", id="keys_as_string"),
pytest.param( ["path/data.txt"], id="keys_as_list"),
],
)
```
##########
providers/google/tests/unit/google/cloud/transfers/test_s3_to_gcs.py:
##########
@@ -306,14 +302,12 @@ def test_get_openlineage_facets_on_start(
class TestS3ToGoogleCloudStorageOperatorDeferrable:
@mock.patch("airflow.providers.google.cloud.transfers.s3_to_gcs.CloudDataTransferServiceHook")
@mock.patch("airflow.providers.google.cloud.transfers.s3_to_gcs.S3Hook")
- @mock.patch("airflow.providers.amazon.aws.operators.s3.S3Hook")
@mock.patch("airflow.providers.google.cloud.transfers.s3_to_gcs.GCSHook")
- def test_execute_deferrable(self, mock_gcs_hook, mock_s3_super_hook,
mock_s3_hook, mock_transfer_hook):
+ def test_execute_deferrable(self, mock_gcs_hook, mock_s3_hook,
mock_transfer_hook):
mock_gcs_hook.return_value.project_id = PROJECT_ID
- mock_list_keys = mock.MagicMock()
- mock_list_keys.return_value = MOCK_FILES
- mock_s3_super_hook.return_value.list_keys = mock_list_keys
+ mock_s3_super_hook = mock.MagicMock()
+ mock_s3_super_hook.list_keys.return_value = MOCK_FILES
Review Comment:
This gets repeated a handful of times, we should consider moving it to a
fixture in the future.
##########
providers/amazon/src/airflow/providers/amazon/aws/operators/s3.py:
##########
@@ -51,38 +52,40 @@ class S3CreateBucketOperator(BaseOperator):
:param bucket_name: This is bucket name you want to create
:param aws_conn_id: The Airflow connection used for AWS credentials.
- If this is None or empty then the default boto3 behaviour is used. If
+ If this is ``None`` or empty then the default boto3 behaviour is used.
If
running Airflow in a distributed manner and aws_conn_id is None or
empty, then default boto3 configuration would be used (and must be
maintained on each worker node).
- :param region_name: AWS region_name. If not specified fetched from
connection.
+ :param region_name: AWS region_name. If not specified then the default
boto3 behaviour is used.
+ :param verify: Whether or not to verify SSL certificates. See:
+
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html
+ :param botocore_config: Configuration dictionary (key-values) for botocore
client. See:
+
https://botocore.amazonaws.com/v1/documentation/api/latest/reference/config.html
"""
- template_fields: Sequence[str] = ("bucket_name",)
+ template_fields: Sequence[str] = aws_template_fields(
+ "bucket_name",
+ )
Review Comment:
Nittiest of nitpicks: here and below, if you drop the trailing comma then
`ruff` should make this a 1-liner
--
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]
