Copilot commented on code in PR #62424:
URL: https://github.com/apache/airflow/pull/62424#discussion_r3033276202
##########
providers/google/tests/unit/google/cloud/operators/test_gcs.py:
##########
@@ -128,8 +128,8 @@ def test_delete_objects(self, mock_hook):
mock_hook.return_value.list.assert_not_called()
mock_hook.return_value.delete.assert_has_calls(
calls=[
- mock.call(bucket_name=TEST_BUCKET, object_name=MOCK_FILES[0]),
- mock.call(bucket_name=TEST_BUCKET, object_name=MOCK_FILES[1]),
+ mock.call(bucket_name=TEST_BUCKET, object_name=MOCK_FILES[0],
ignore_error=False),
+ mock.call(bucket_name=TEST_BUCKET, object_name=MOCK_FILES[1],
ignore_error=False),
],
Review Comment:
`ignore_error` is a new operator parameter, but the unit tests here only
cover the default `ignore_error=False` path. Please add a test case that
instantiates `GCSDeleteObjectsOperator(ignore_error=True)` and asserts the hook
is called with `ignore_error=True` so the new behavior is covered and guarded
against regressions.
##########
providers/google/src/airflow/providers/google/cloud/hooks/gcs.py:
##########
@@ -705,22 +705,27 @@ def is_older_than(self, bucket_name: str, object_name:
str, seconds: int) -> boo
return True
return False
- def delete(self, bucket_name: str, object_name: str) -> None:
+ def delete(self, bucket_name: str, object_name: str, ignore_error: bool =
False) -> None:
"""
Delete an object from the bucket.
:param bucket_name: name of the bucket, where the object resides
:param object_name: name of the object to delete
+ :param ignore_error: (Optional) whether to ignore NotFound exceptions.
Default: False
"""
client = self.get_conn()
bucket = client.bucket(bucket_name)
blob = bucket.blob(blob_name=object_name)
- blob.delete()
- get_hook_lineage_collector().add_input_asset(
- context=self, scheme="gs", asset_kwargs={"bucket": bucket.name,
"key": blob.name}
- )
-
- self.log.info("Blob %s deleted.", object_name)
+ try:
+ blob.delete()
+ get_hook_lineage_collector().add_input_asset(
+ context=self, scheme="gs", asset_kwargs={"bucket":
bucket.name, "key": blob.name}
+ )
+ self.log.info("Blob %s deleted.", object_name)
+ except NotFound:
+ self.log.warning("Blob %s in bucket %s does not exist.",
blob.name, bucket.name)
+ if not ignore_error:
+ raise
Review Comment:
PR description says the NotFound suppression is implemented by passing a
no-op `on_error` lambda to `Bucket.delete_blobs`, but the actual implementation
changes `GCSHook.delete()` to wrap `blob.delete()` in a `try/except NotFound`.
Please update the PR description (or adjust the implementation) so they match,
since this impacts how reviewers/users understand the behavior and API surface.
--
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]
