vincbeck commented on code in PR #33031:
URL: https://github.com/apache/airflow/pull/33031#discussion_r1328929262
##########
airflow/providers/amazon/aws/transfers/gcs_to_s3.py:
##########
@@ -117,7 +119,23 @@ def __init__(
) -> None:
super().__init__(**kwargs)
- self.bucket = bucket
+ if bucket and gcs_bucket:
Review Comment:
Do we need this check? If both are provided, we should use `gcs_bucket` IMO.
To me we should do (pseudo code):
```python
if bucket:
warnings.warn()
self.gcs_bucket = bucket
if gcs_bucket:
self.gcs_bucket = gcs_bucket
```
As such, if both are provided, `gcs_bucket` is used
##########
airflow/providers/amazon/aws/transfers/gcs_to_s3.py:
##########
@@ -117,7 +119,23 @@ def __init__(
) -> None:
super().__init__(**kwargs)
- self.bucket = bucket
+ if bucket and gcs_bucket:
+ raise ValueError("You must pass either bucket or gcs_bucket, but
not both.")
+ elif bucket:
+ warnings.warn(
+ "The 'bucket' parameter is deprecated and will be removed in a
future version. "
+ "Please use 'gcs_bucket' instead.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
+ self.gcs_bucket = bucket
+ elif gcs_bucket:
+ self.gcs_bucket = gcs_bucket
+ else:
+ raise ValueError("You must pass either bucket or gcs_bucket.")
+ # The bucket attribute is unused, but must be set on the operator
instance in
+ # order to include it in template_fields.
+ self.bucket = None
Review Comment:
No longer needed?
##########
tests/providers/amazon/aws/transfers/test_gcs_to_s3.py:
##########
@@ -196,6 +196,50 @@ def test_execute(self, mock_hook):
assert sorted(MOCK_FILES) == sorted(uploaded_files)
assert sorted(MOCK_FILES) == sorted(hook.list_keys("bucket",
delimiter="/"))
+ @mock.patch("airflow.providers.amazon.aws.transfers.gcs_to_s3.GCSHook")
+ def test_execute_gcs_bucket_rename_compatibility(self, mock_hook):
Review Comment:
Love that
##########
tests/providers/amazon/aws/transfers/test_gcs_to_s3.py:
##########
@@ -57,7 +57,7 @@ def test_execute__match_glob(self, mock_hook):
operator = GCSToS3Operator(
task_id=TASK_ID,
- bucket=GCS_BUCKET,
+ gcs_bucket=GCS_BUCKET,
Review Comment:
Nice :)
##########
airflow/providers/amazon/aws/transfers/gcs_to_s3.py:
##########
@@ -41,7 +41,8 @@ class GCSToS3Operator(BaseOperator):
For more information on how to use this operator, take a look at the
guide:
:ref:`howto/operator:GCSToS3Operator`
- :param bucket: The Google Cloud Storage bucket to find the objects.
(templated)
+ :param gcs_bucket: The Google Cloud Storage bucket to find the objects.
(templated)
+ :param bucket: (Deprecated) Use gcs_bucket instead.
Review Comment:
```suggestion
:param bucket: (Deprecated) Use ``gcs_bucket`` instead.
```
--
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]
