Skip to content
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

Update checksums.rb #2890

Closed
wants to merge 2 commits into from
Closed

Conversation

PatoSoft
Copy link

@PatoSoft PatoSoft commented Aug 7, 2023

Hi!

Thank you for your great job!

Headers are missing :complete_multipart_upload so I need to calculate it outside the plugin. I don't know if this is intended but it is the oposite of what I understand by ready this file comments.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Hi!

Thank you for your great job!

Headers are missing :complete_multipart_upload so I need to calculate it outside the plugin. I don't know if this is intended but it is the oposite of what I understand by ready this file comments.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@mullermp
Copy link
Contributor

mullermp commented Aug 7, 2023

Yep, that looks to be a mistake that's been around for a long time quite possibly.. Were you able to confirm that this change succeeds?

@mullermp
Copy link
Contributor

mullermp commented Aug 7, 2023

I'm not too sure about this change working. The plugin looks to be computing checksums on the request body. Compete Multipart Upload does not take a body here, so the calculated TreeHash is always the same digest.

@mullermp
Copy link
Contributor

mullermp commented Aug 7, 2023

Unfortunately I think you'll need to manage the TreeHash yourself - this looks to have been intended. I think you could either construct a TreeHash and read the entire file in 1 MB chunks like the documentation suggests and stick it at the end of CompleteMultipartUpload:

    #    tree_hash = TreeHash.new
    #    tree_hash.update(file.read(1024 * 1024)) until file.eof?
    #    tree_hash.digest

.. or for each UploadMultipartPart output's checksum, append it to an array and assign it to TreeHash's hashes accessor and then call digest.

I think we could possibly consider this as a feature request for creating an abstraction class for Glacier multipart uploads, similar to how we manage S3 uploads and download file through Aws::S3::Object.upload_file(multipart_copy: true).

@mullermp mullermp added feature-request A feature should be added or improved. pr/do-not-merge This PR should not be merged at this time. labels Aug 7, 2023
@PatoSoft
Copy link
Author

PatoSoft commented Aug 8, 2023

Thank you for the information!

I am using Threehash::caculate_tree_hash(body) at the moment.

It would be nice to have that(multipart_copy: true)feature for Glacier.

Thank you!

@mullermp mullermp removed the feature-request A feature should be added or improved. label Oct 24, 2023
@mullermp
Copy link
Contributor

Created an issue in our repo to track this (closing this PR)

@mullermp mullermp closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants