feat(storage): support deleteSourceObjects option in compose sample#14348
feat(storage): support deleteSourceObjects option in compose sample#14348nidhiii-27 wants to merge 1 commit into
Conversation
Add deleteSourceObjects parameter to compose sample. Updated test to assert both delete behavior scenarios. [Generated-by: AI]
There was a problem hiding this comment.
Code Review
This pull request introduces a delete_source_objects parameter to the compose_file snippet, allowing source blobs to be deleted after composition, and adds corresponding unit tests. The feedback suggests refactoring the print statements in storage_compose_file.py to reduce string duplication, and moving the destination blob cleanup inside the with block in the tests to improve scoping and clarity.
| if delete_source_objects: | ||
| print( | ||
| "New composite object {} in the bucket {} was created by combining {} and {}. Source objects were deleted.".format( | ||
| destination_blob_name, bucket_name, first_blob_name, second_blob_name | ||
| ) | ||
| ) | ||
| else: | ||
| print( | ||
| "New composite object {} in the bucket {} was created by combining {} and {}".format( | ||
| destination_blob_name, bucket_name, first_blob_name, second_blob_name | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The print statements for delete_source_objects contain duplicated text. We can simplify this by constructing the base message first and conditionally appending the deletion status. This reduces duplication and improves maintainability.
message = "New composite object {} in the bucket {} was created by combining {} and {}".format(
destination_blob_name, bucket_name, first_blob_name, second_blob_name
)
if delete_source_objects:
message += " Source objects were deleted."
print(message)| with tempfile.NamedTemporaryFile() as dest_file: | ||
| destination = storage_compose_file.compose_file( | ||
| test_bucket.name, | ||
| source_files[0], | ||
| source_files[1], | ||
| dest_file.name, | ||
| delete_source_objects=True, | ||
| ) | ||
| composed = destination.download_as_bytes() | ||
|
|
||
| assert composed.decode("utf-8") == source_files[0] + source_files[1] | ||
| assert not test_bucket.blob(source_files[0]).exists() | ||
| assert not test_bucket.blob(source_files[1]).exists() | ||
|
|
||
| # Clean up destination file | ||
| destination.delete() |
There was a problem hiding this comment.
Similar to the previous test block, the destination blob cleanup should be moved inside the with block to keep the scope localized, and the comment updated to refer to 'destination blob' instead of 'destination file'.
| with tempfile.NamedTemporaryFile() as dest_file: | |
| destination = storage_compose_file.compose_file( | |
| test_bucket.name, | |
| source_files[0], | |
| source_files[1], | |
| dest_file.name, | |
| delete_source_objects=True, | |
| ) | |
| composed = destination.download_as_bytes() | |
| assert composed.decode("utf-8") == source_files[0] + source_files[1] | |
| assert not test_bucket.blob(source_files[0]).exists() | |
| assert not test_bucket.blob(source_files[1]).exists() | |
| # Clean up destination file | |
| destination.delete() | |
| with tempfile.NamedTemporaryFile() as dest_file: | |
| destination = storage_compose_file.compose_file( | |
| test_bucket.name, | |
| source_files[0], | |
| source_files[1], | |
| dest_file.name, | |
| delete_source_objects=True, | |
| ) | |
| composed = destination.download_as_bytes() | |
| assert composed.decode("utf-8") == source_files[0] + source_files[1] | |
| assert not test_bucket.blob(source_files[0]).exists() | |
| assert not test_bucket.blob(source_files[1]).exists() | |
| # Clean up destination blob | |
| destination.delete() |
Add deleteSourceObjects parameter to compose sample.
Updated test to assert both delete behavior scenarios.