Skip to content

Correctly handle blob MD5 sums in custom blob resource, need to be base64-encoded#3718

Merged
thomas11 merged 2 commits into
masterfrom
tkappler/blob-md5
Nov 20, 2024
Merged

Correctly handle blob MD5 sums in custom blob resource, need to be base64-encoded#3718
thomas11 merged 2 commits into
masterfrom
tkappler/blob-md5

Conversation

@thomas11

Copy link
Copy Markdown
Contributor

This was initially missed because the previously used Azure SDK in custom_storage.go, and the newer one used in custom_storage_azidentity.go, behave differently. The older SDK leaves the user-provided MD5 sum alone, so the user needs to provide it base64-encoded, as the integration test examples/static-website/index.ts does. The newer SDK, however, expects the MD5 sum as a raw byte array and does the base64-decoding internally. Providing an already encoded sum leads to double-encoding and an invalid sum being sent to Azure.

@thomas11 thomas11 requested review from a team and danielrbradley November 20, 2024 09:35
@thomas11 thomas11 self-assigned this Nov 20, 2024
@github-actions

Copy link
Copy Markdown
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@danielrbradley danielrbradley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change looks good. Would be good to have an integration test for the blobs running ahead of merge at some point.

Options I can see here:

  1. Assuming you've run the test locally, that should suffice for now.
  2. We could also do a manual CI run for the branch to include all tests.
  3. We could also unmark one of the integration tests with skipIfShort so it's always run.
  4. Or we should introduce a merge queue so it only merges after the full integration test suite has run.

@codecov

codecov Bot commented Nov 20, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 56.30%. Comparing base (5713385) to head (29dcfee).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...urces/customresources/custom_storage_azidentity.go 28.57% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3718      +/-   ##
==========================================
- Coverage   56.31%   56.30%   -0.02%     
==========================================
  Files          74       74              
  Lines       11735    11738       +3     
==========================================
  Hits         6609     6609              
- Misses       4635     4638       +3     
  Partials      491      491              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@thomas11

Copy link
Copy Markdown
Contributor Author

Change looks good. Would be good to have an integration test for the blobs running ahead of merge at some point.

Options I can see here:

  1. Assuming you've run the test locally, that should suffice for now.
  2. We could also do a manual CI run for the branch to include all tests.
  3. We could also unmark one of the integration tests with skipIfShort so it's always run.
  4. Or we should introduce a merge queue so it only merges after the full integration test suite has run.

I ran the full test suite via gh workflow run acceptance-test.yml --ref tkappler/blob-md5, result here.

@thomas11 thomas11 merged commit 93fac42 into master Nov 20, 2024
@thomas11 thomas11 deleted the tkappler/blob-md5 branch November 20, 2024 11:22
@pulumi-bot

Copy link
Copy Markdown
Contributor

This PR has been shipped in release v2.73.0.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants