-
-
Notifications
You must be signed in to change notification settings - Fork 389
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
Minio : pass size argument #1489
Conversation
4c9a4ec
to
bb0e5cc
Compare
I tried to add the same behavior to the ExternalStorageAttachmentStore's upload method so that I could add a test here before realizing that there was no reliable way to know the size of the the readable stream coming in, duh 🙃 |
I tried to add a test to https://github.com/gristlabs/grist-core/blob/main/test/server/lib/HostedStorageManager.ts and failed miserably, this JS code is a bit above my paygrade 😶 help appreciated here ! |
I am fairly confident that the failed CI run is unrelated to my changes |
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.
Also I wonder if whether should add tests after these changes.
Ok, about the second comment, but what about the first one ? |
Done ✅ |
These were usefull to me when I read the code. Hopefully they will help future readers as well :)
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.
Thanks @vviers ! I think existing tests cover this, I'd have expected failures if the introduction of size caused an issue.
Fixes #1488
Looking at the MinIO.js client's code for putObject, it looks like passing the
size
argument lets it be clever about whether to perform an upload in one part or as a multi-part.I also threw some new comments in the codebase that were usefull to me when reading the code. Can revert or shove them in a separate PR if you'd prefer.