Skip to content

feat(storage): Making crc32c default for download#34002

Merged
v-pratap merged 4 commits into
googleapis:mainfrom
shubhangi-google:making_crc32c_defalut_for_download
Jun 12, 2026
Merged

feat(storage): Making crc32c default for download#34002
v-pratap merged 4 commits into
googleapis:mainfrom
shubhangi-google:making_crc32c_defalut_for_download

Conversation

@shubhangi-google

@shubhangi-google shubhangi-google commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

This pull request changes the default verification method for file downloads from MD5 to CRC32C across the google-cloud-storage library and updates the corresponding tests.

@shubhangi-google shubhangi-google requested review from a team and yoshi-approver as code owners June 2, 2026 04:55
@shubhangi-google shubhangi-google marked this pull request as draft June 2, 2026 04:55
@shubhangi-google shubhangi-google changed the title Making crc32c defalut for download feat(storage): Making crc32c defalut for download Jun 2, 2026
@shubhangi-google shubhangi-google marked this pull request as ready for review June 2, 2026 07:37
@aandreassa aandreassa added the api: storage Issues related to the Cloud Storage API. label Jun 2, 2026
@kalragauri kalragauri requested a review from v-pratap June 3, 2026 06:34
@@ -1098,7 +1098,7 @@ def update generation: nil,
# downloaded.rewind
# downloaded.read #=> "world"
#
def download path = nil, verify: :md5, encryption_key: nil, range: nil,
def download path = nil, verify: :crc32c, encryption_key: nil, range: nil,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please ensure the official documentation for checksumming is updated to reflect that :crc32c is now the default instead of :md5

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -320,7 +320,7 @@ def file.md5
end

describe "verified downloads" do
it "verifies m5d by default" do
it "verifies crc32c by default" do

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How it was working before, it was using m5d instead of md5? Does this test validate the actual use case?

@shubhangi-google shubhangi-google Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this part is just description of test case which would have been typed incorrectly

def file.md5
"1B2M2Y8AsgTpgAmY7PhCfg=="
# Stub the crc32c to match.
def file.crc32c

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you also add non empty file verification?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@shubhangi-google shubhangi-google requested a review from v-pratap June 4, 2026 06:55
@kalragauri kalragauri changed the title feat(storage): Making crc32c defalut for download feat(storage): Making crc32c default for download Jun 5, 2026
@kalragauri kalragauri added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 5, 2026
@v-pratap v-pratap merged commit 8662ebd into googleapis:main Jun 12, 2026
17 of 20 checks passed
@github-actions github-actions Bot added the release-please:force-run To run release-please label Jun 12, 2026
@release-please release-please Bot removed the release-please:force-run To run release-please label Jun 12, 2026
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