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

Accept multipart_upload parameters (supports ServerSideEncryption) for S3 #202

Merged
merged 4 commits into from
Jun 22, 2018

Conversation

eschwartz
Copy link
Contributor

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.

smart_open('s3://path/to/file', 'wb', multipart_upload={
  'ServerSideEncryption': 'AES256'
})

@eschwartz
Copy link
Contributor Author

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()

@eschwartz eschwartz changed the title S3: Accept multipart_upload parameters S3: Accept multipart_upload parameters (supports ServerSideEncryption) Jun 18, 2018
@eschwartz
Copy link
Contributor Author

I borrowed test coverage from #196, which apparently does the same thing as this PR.

@eschwartz
Copy link
Contributor Author

I figured out how to add unit tests for this.

I also renamed the argument to s3_upload, in consideration of @mpenkov's comments here. It also gives us flexibility to add a s3_download argument later, to support args for S3#get

@eschwartz
Copy link
Contributor Author

@mpenkov or @piskvorky -- Can we get this merged?

Copy link
Owner

@piskvorky piskvorky left a 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!

)



Copy link
Owner

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.

Copy link
Contributor Author

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'
Copy link
Owner

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@eschwartz
Copy link
Contributor Author

Requested changes were made.

@piskvorky
Copy link
Owner

LGTM.

@menshikh-iv
Copy link
Contributor

Looks good for me too 👍, ping @mpenkov

@menshikh-iv menshikh-iv changed the title S3: Accept multipart_upload parameters (supports ServerSideEncryption) Accept multipart_upload parameters (supports ServerSideEncryption) for S3 Jun 22, 2018
@menshikh-iv
Copy link
Contributor

Thanks @eschwartz congratz with your first contribution 🥇

@menshikh-iv menshikh-iv merged commit 4e90ec7 into piskvorky:master Jun 22, 2018
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