Skip to content

Conversation

@shubhangi-google
Copy link
Contributor

@shubhangi-google shubhangi-google commented Aug 21, 2025

This change sets Checksum CRC32C as default when we are uploading a file to bucket

@shubhangi-google shubhangi-google marked this pull request as ready for review August 21, 2025 12:55
@shubhangi-google shubhangi-google changed the title feat: setting default checksum feat(storage): setting default checksum Aug 26, 2025
@aandreassa aandreassa added the api: storage Issues related to the Cloud Storage API. label Nov 21, 2025
new_file_name = random_file_path
new_file_contents = StringIO.new "Hello world"
mock = Minitest::Mock.new
mock.expect :insert_object, create_file_gapi(bucket.name, new_file_name),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are we verifying the checksum for the new String IO object here, also use a slightly different string in different tests for robustness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow missed passing CRC32c value, fixed it now

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not explicitly calculate the checksum value here as well!

@shubhangi-google
Copy link
Contributor Author

FYI: @cpriti-os the KOKORO failures is not related to changes done by us

new_file_name = random_file_path
new_file_contents = StringIO.new

new_file_contents = StringIO.new("Hello world strngio")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like typo

new_file_name = random_file_path
new_file_contents = StringIO.new "Hello world"
mock = Minitest::Mock.new
mock.expect :insert_object, create_file_gapi(bucket.name, new_file_name),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not explicitly calculate the checksum value here as well!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to modify this documentation for the new default checksumming, make appropriate changes here, to the method summary and examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants