Skip to content

fix 4MB index-cache.yaml limit with Azure Blob Storage#667

Merged
cbuto merged 3 commits into
chartmuseum:mainfrom
yuanjumao:main
Jun 26, 2023
Merged

fix 4MB index-cache.yaml limit with Azure Blob Storage#667
cbuto merged 3 commits into
chartmuseum:mainfrom
yuanjumao:main

Conversation

@yuanjumao

Copy link
Copy Markdown
Contributor

Currently, this implementation does not use the azure API's chunked upload, so we will always only be able to upload blobs smaller than 4MB. This PR adds chunked uploads that allow more than 4MB.

Ref: https://learn.microsoft.com/en-us/rest/api/storageservices/append-block, limited by request header Content-Length

@cbuto

cbuto commented Mar 24, 2023

Copy link
Copy Markdown
Collaborator

thanks for the PR @yuanjumao! can you rebase with main please?

@yuanjumao

Copy link
Copy Markdown
Contributor Author

@cbuto Okay, I've already done a rebase with main

@cbuto cbuto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I finally got to do some testing, overall it looks like this solves the 4 MB upload issue.

just one minor nit about the variable naming

Comment thread microsoft.go Outdated
@cbuto

cbuto commented Jun 15, 2023

Copy link
Copy Markdown
Collaborator

@scbizu do you want to have a second look here? I ran this locally and uploads worked fine, plus I tested with uploading a large chart.

@scbizu

scbizu commented Jun 17, 2023

Copy link
Copy Markdown
Collaborator

LGTM , but if we migrate the dependency , this PR changes will still work ?

@cbuto

cbuto commented Jun 20, 2023

Copy link
Copy Markdown
Collaborator

@scbizu I think it would based on a brief glance but i think we'll just have to cross that bridge when we migrate to the new module

@scbizu

scbizu commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

@cbuto Yes, definitely . Let's raise another PR to migrate that and involve some testing .

@cbuto

cbuto commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

@scbizu it might be a decent sized effort to migrate to the new module (i don't have much context around the azure SDK/API), maybe we should get this in and deal with the migration later on

@scbizu

scbizu commented Jun 24, 2023

Copy link
Copy Markdown
Collaborator

@cbuto ok , let's merge this PR to fix the temporary situation .

Co-authored-by: Casey Buto <[email protected]>
Comment thread microsoft.go Outdated
change var name to content
@cbuto
cbuto merged commit 3bb41a8 into chartmuseum:main Jun 26, 2023
@cbuto

cbuto commented Jun 26, 2023

Copy link
Copy Markdown
Collaborator

thanks @yuanjumao! i'll create a release shortly and bump the module in chartmuseum 🙏

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