-
-
Notifications
You must be signed in to change notification settings - Fork 381
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
Accept multipart_upload
parameters (supports ServerSideEncryption) for S3
#202
Accept multipart_upload
parameters (supports ServerSideEncryption) for S3
#202
Conversation
Again, I wasn't sure how to test this. The integration test is from #194. Here's the script I ran locally to validate my changes: import os
import random
import string
import boto3
os.sys.path.insert(0, './')
from smart_open import smart_open
def main():
uid = ''.join(random.choice(string.ascii_letters) for n in range(12))
bucket = '[bucket name]'
key = f"test_file_{uid}"
s3_uri = f"s3://{bucket}/{key}"
print(f"Writing to {s3_uri}")
write_stream = smart_open(
s3_uri, 'wb', encoding='utf-8',
multipart_upload={
'ServerSideEncryption': 'AES256'
}
)
with write_stream as write_stream:
write_stream.write("test data")
# Check that the file is encrypted
res = boto3.client('s3').get_object(
Bucket=bucket,
Key=key
)
assert res['ServerSideEncryption'] == 'AES256'
main() |
multipart_upload
parametersmultipart_upload
parameters (supports ServerSideEncryption)
I borrowed test coverage from #196, which apparently does the same thing as this PR. |
I figured out how to add unit tests for this. I also renamed the argument to |
19ea4fd
to
e0877c2
Compare
@mpenkov or @piskvorky -- Can we get this merged? |
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.
Minor doc nit picks, otherwise looks good!
smart_open/tests/test_smart_open.py
Outdated
) | ||
|
||
|
||
|
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.
PEP8: two blank lines between module-level classes or functions.
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.
fixed
README.rst
Outdated
@@ -100,8 +100,14 @@ There are a few optional keyword arguments that are useful only for S3 access. | |||
|
|||
>>> smart_open.smart_open('s3://', host='s3.amazonaws.com') | |||
>>> smart_open.smart_open('s3://', profile_name='my-profile') | |||
>>> smart_open.smart_open('s3://', s3_upload={ | |||
'ServerSideEncryption': 'AES256' |
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.
One-liner please (like the other examples).
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.
fixed
Requested changes were made. |
LGTM. |
Looks good for me too 👍, ping @mpenkov |
multipart_upload
parameters (supports ServerSideEncryption)multipart_upload
parameters (supports ServerSideEncryption) for S3
Thanks @eschwartz congratz with your first contribution 🥇 |
Accept a
multipart_upload
parameter for S3 operations. Accepts the same arguments as S3.initiate_multipart_upload. Allows for, among other things, using S3 server-side encryption.eg.