-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix 4MB index-cache.yaml limit with Azure Blob Storage #667
Conversation
thanks for the PR @yuanjumao! can you rebase with main please? |
@cbuto Okay, I've already done a rebase with main |
There was a problem hiding this comment.
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
@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. |
LGTM , but if we migrate the dependency , this PR changes will still work ? |
@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 |
@cbuto Yes, definitely . Let's raise another PR to migrate that and involve some testing . |
@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 |
@cbuto ok , let's merge this PR to fix the temporary situation . |
Co-authored-by: Casey Buto <[email protected]>
change var name to content
thanks @yuanjumao! i'll create a release shortly and bump the module in chartmuseum 🙏 |
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