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

Fix S3 resource identifier order bug #62

Merged
merged 3 commits into from
Feb 10, 2015
Merged

Fix S3 resource identifier order bug #62

merged 3 commits into from
Feb 10, 2015

Conversation

danielgtaylor
Copy link
Member

This change is for the issue described in #59 and updates the model based on
the output of the proposed change in amazon-archives/aws-model-validators#2. The
following now works properly:

import boto3

obj = boto3.resource('s3').Bucket('foo').Object('bar')
mpu = obj.initiate_multipart_upload()
part = mpu.MultipartUploadPart(1)
print(part.object_key)
# => 'foo'

cc @jamesls @kyleknap

@danielgtaylor danielgtaylor added the bug This issue is a confirmed bug. label Feb 9, 2015
@danielgtaylor danielgtaylor self-assigned this Feb 9, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.58% when pulling 08df735c0a5fa6f8a851e96a622e696b4d5d20bc on s3-ident-fix into f04f5e3 on develop.

@kyleknap
Copy link
Contributor

Would there be any way to add a test for this? Maybe an integration test? I would suggest just creating a multipart upload, uploading a part, and then listing the parts for the assertion. You do not need to do a full multipart upload as that requires more than 5 MB data transferred.

@danielgtaylor
Copy link
Member Author

@kyleknap there is a test added here:

https://github.com/awslabs/aws-model-validators/pull/2/files#diff-e4bf191bb600a1c7130f57d133bf1e5e

Part of the advantage of adding it to the validator is that each SDK doesn't need to duplicate the effort. Do you think we should still add some extra test for this in Boto3?

@kyleknap
Copy link
Contributor

I would say an integration for multipart upload in boto3 would be nice. Its a fairly used operation and it would not take too much time (when running the test) to instantiate a multipart upload, upload a single small (under < 5MB) part, and then complete the multipart upload. And that should succeed because there is only one part involved in the multipart upload.

This change fixes the issue described in #59 by updating the model based on
the output of the proposed change in amazon-archives/aws-model-validators#2. The
following now works properly:

```python
import boto3

obj = boto3.resource('s3').Bucket('foo').Object('bar')
mpu = obj.initiate_multipart_upload()
part = mpu.MultipartUploadPart(1)
print(part.object_key)
```
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.61% when pulling aaab03e on s3-ident-fix into 28ae6a8 on develop.

}

mpu.complete(MultipartUpload=part_info)
self.addCleanup(bucket.Object('mp-test.txt').delete)
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't assert anything. What about downloading the object and verifying its contents are b'hello, world!'?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.61% when pulling df4d95b on s3-ident-fix into 28ae6a8 on develop.

@kyleknap
Copy link
Contributor

Thanks for adding the integration test. 🚢

@jamesls
Copy link
Member

jamesls commented Feb 10, 2015

:shipit:

danielgtaylor added a commit that referenced this pull request Feb 10, 2015
Fix S3 resource identifier order bug
@danielgtaylor danielgtaylor merged commit 881304d into develop Feb 10, 2015
@danielgtaylor danielgtaylor deleted the s3-ident-fix branch February 10, 2015 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants