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

Bump aws-c-s3 to v0.2.9 #228

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

jamesbornholt
Copy link
Member

This picks up the fix for #218.

Signed-off-by: James Bornholt bornholt@amazon.com


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

This picks up the fix for awslabs#218.

Signed-off-by: James Bornholt <bornholt@amazon.com>
@jamesbornholt jamesbornholt temporarily deployed to PR integration tests May 1, 2023 17:02 — with GitHub Actions Inactive
@jamesbornholt jamesbornholt temporarily deployed to PR integration tests May 1, 2023 17:02 — with GitHub Actions Inactive
@jamesbornholt jamesbornholt temporarily deployed to PR integration tests May 1, 2023 17:02 — with GitHub Actions Inactive
@jamesbornholt jamesbornholt marked this pull request as ready for review May 1, 2023 17:32
Copy link
Contributor

@sauraank sauraank May 2, 2023

Choose a reason for hiding this comment

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

What is the process to upgrade the crt for our Mountpoint? I got the changes that are in the update like yml files issue template and workflow on github and changes to include the case of bytes = "0-0".
And there is a test for it as well, so that should be fine.
#226 should pass now I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question. Would it be helpful if the PR (maybe an automated comment) would point you to this document?

https://github.com/awslabs/mountpoint-s3/blob/main/mountpoint-s3-crt-sys/UPDATING_CRT.md

Also, does it feel like there's information missing?

I think what we really need to do is review the commit history (not the diff), and if there's anything that sounds relevant for us - we should dig deeper and check there's no changes we care about. For example, we should worry if the lifetime (in C) changes for a parameter we pass in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, got it. The document explains the process for updating the submodule, not the reviewer right?
Yes, for the first time reviewer an automated comment about the document will help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe we can add some pointers for what a reviewer should look for.

@jamesbornholt jamesbornholt merged commit 9774734 into awslabs:main May 2, 2023
@jamesbornholt jamesbornholt deleted the bump-crt-s3 branch May 2, 2023 15:46
passaro added a commit that referenced this pull request May 4, 2023
Improve PartQueue to report an error (rather than blocking indeterminately) when trying to read beyond the body returned by the GetObject request. 

The previous behavior, compounded with a bug in the CRT (now fixed: #228) where GetObject unexpectedly returned an empty body, manifested in #218.
---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
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