SameerMesiah97 commented on code in PR #61463:
URL: https://github.com/apache/airflow/pull/61463#discussion_r2770496182
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
I would suggest using `posixpath.join(dest_gcs_prefix, obj)` as the behavior
of `os.path.join` is OS dependent. On windows, it can result in the `\\`
separator being used during construction, resulting in an invalid path. Now, it
won't cause issues in 99% of cases but for the odd contributor running this
code locally on their windows machine, `os.path.join` can definitely cause
issues. This is more of a nit though.
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
It looks like `dest_gcs_bucket, dest_gcs_prefix =
_parse_gcs_url(self.dest_gcs)` will return the same value regardless of the
iteration as `self.dest_gcs` is constant. Any reason why it can't be moved
outside the loop?
##########
providers/google/tests/unit/google/cloud/transfers/test_adls_to_gcs.py:
##########
@@ -99,8 +98,18 @@ def test_execute(self, gcs_mock_hook, adls_one_mock_hook,
adls_two_mock_hook):
impersonation_chain=IMPERSONATION_CHAIN,
)
- # we expect MOCK_FILES to be uploaded
- assert sorted(MOCK_FILES) == sorted(uploaded_files)
+ # Verify that the return value is a list of destination GCS URIs
+ assert isinstance(uploaded_files, list)
+ assert len(uploaded_files) == len(MOCK_FILES)
+
+ # Each returned value should be a GCS URI starting with gs://
+ for uri in uploaded_files:
+ assert uri.startswith("gs://")
Review Comment:
This is redundant as the assertion on the next line already checks for uris
starting with `gs://`. I would advise you to remove it.
##########
providers/google/tests/unit/google/cloud/transfers/test_adls_to_gcs.py:
##########
@@ -138,5 +146,15 @@ def test_execute_with_gzip(self, gcs_mock_hook,
adls_one_mock_hook, adls_two_moc
any_order=True,
)
- # we expect MOCK_FILES to be uploaded
- assert sorted(MOCK_FILES) == sorted(uploaded_files)
+ # Verify that the return value is a list of destination GCS URIs
+ assert isinstance(uploaded_files, list)
+ assert len(uploaded_files) == len(MOCK_FILES)
+
+ # Each returned value should be a GCS URI starting with gs://
+ for uri in uploaded_files:
+ assert uri.startswith("gs://")
Review Comment:
Redundant assertion here as well. See the above comment.
--
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]
