-
Notifications
You must be signed in to change notification settings - Fork 32
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
Implement S3 storage class for Streaming and External basic transfer adapters #81
Conversation
To get this done properly a 3rd party pypi package is required, e.g. boto3-stubs
a406ef8
to
885fa2d
Compare
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.
I think this is great! I have a few comments, I don't think any of them is huge, but would be good to fix or get your feedback on them. I think the requirements.txt formatting is a result of an older version of pip-tools
- try to upgrade to the latest and see what happens.
466c7ee
to
65f4c5f
Compare
Thanks @shevron . I've fixed/updated all listed issues. I have left two conversations unresolved to get your feedback on:
|
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.
Getting very close now 👍
giftless/storage/amazon_s3.py
Outdated
raw_filename = extra.get('filename') if extra else oid | ||
assert raw_filename | ||
filename = safe_filename(raw_filename) | ||
params = { | ||
'Bucket': self.bucket_name, | ||
'Key': self._get_blob_path(prefix, oid), | ||
'ResponseContentDisposition': f"attachment;filename={filename}" | ||
} |
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.
I think it would be nicer to not set content-disposition unless we need to (that is unless the user specified a custom name).
Something like:
raw_filename = extra.get('filename') if extra else oid | |
assert raw_filename | |
filename = safe_filename(raw_filename) | |
params = { | |
'Bucket': self.bucket_name, | |
'Key': self._get_blob_path(prefix, oid), | |
'ResponseContentDisposition': f"attachment;filename={filename}" | |
} | |
params = {'Bucket': self.bucket_name, 'Key': self._get_blob_path(prefix, oid)} | |
if extra and 'filename' in extra: | |
filename = safe_filename(extra['filename']) | |
params['ResponseContentDisposition'] = f'attachment; filename="{filename}"' |
I think this will also pass type checking.
I've also added double quotes around the filename in the header, it is not strictly required unless the filename contains a space or a semicolon IIRC. While right now safe_filename
prevents that, in the future I'd like to make it less strict and then it will be required.
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.
That's helpful, thank you. I have manually verified and actually the 'ResponseContentDisposition'
can only be provided if we want a custom filename for downloaded file. Otherwise it defaults to object it oid
.
Type checks also passed this change. Thank you for fixing the syntax of the header too.
I have changed only the params
dict init so it aligns better with the rest of the dicts in the class.
Co-authored-by: Shahar Evron <[email protected]>
This is great feedback. Thanks as I've learned something new because of it :) |
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.
Looks very good, thanks.
@tomeksabala this looks great now, I'm merging this to master, and will issue a new release with this included in the next few days - I want to ensure the docs are up to date, and run the full test suite with a live AWS account before I make an official "stable" release. |
/cc @rufuspollock |
@tomeksabala @shevron 👏 👏 |
Implements a feature described in issue #3
I have created S3 storage with class
giftless.storage.aws_s3.AwsS3Storage
implementing two interfaces:StreamingStorage
andExternalStorage
.I have decided to support AWS authentication in a native way to boto3. In my opinion AWS users are used to the most common ways to provide credentials as I've explained in
docs/source/storage-backends.md:127
One outstanding issue is the
requirement.txt
file in this PR. I needed to addboto3
andbotocore
requirements torequirements.in
. After runningmake requirements.txt
I couldn't keep the old formatting in place. I can't fix it once I'm told how to do this properly.