liormizr / s3path

s3path is a pathlib extension for AWS S3 Service
Apache License 2.0
207 stars 39 forks source link

Config arguments such as ACL not passed to copy #73

Closed maresb closed 3 years ago

maresb commented 3 years ago

The rename() method calls Bucket.copy() which does not accept configuration arguments as normal kwargs. Instead, the configuration arguments should be passed in as ExtraArgs. (reference)

As a result, if I have ACL in my config, the result of a rename() operation fails to apply the correct ACL to my renamed file.

liormizr commented 3 years ago

Hi @maresb Thanks for the issue.

Just so I'll understand the issue. In boto3 some methods require you to specify ACL like this for example:

response = bucket.create(
    ACL='private'|'public-read'|'public-read-write'|'authenticated-read',
    ...)

But some methods need you to specify it inside a ExtraArgs dict like this?


bucket.copy(copy_source, 'otherkey', ExtraArgs={'ACL': 'private'|'public-read'|'public-read-write'|'authenticated-read'},
    ...)

That's the issue?

maresb commented 3 years ago

Exactly.

liormizr commented 3 years ago

How I love Boto3... ;-)

What about this solution? will this work?

>>> from s3path import S3Path, register_configuration_parameter
>>> bucket = S3Path('/my-bucket/')
>>> register_configuration_parameter(bucket, parameters={'ACL': '...', 'ExtraArgs': {'ACL': '...'})

I know that it;s not the cleanest, but will this solution will work? If yes' maybe we can Always update ExtraArgs accordingly Or they do validation on the keys there?

maresb commented 3 years ago

I considered this as well. And you're correct, there is additional validation in ExtraArgs. A function will fail when given ExtraArgs which are not allowed.

Unfortunately there seems to be no systematic way to grab the all the allowed arguments for a given function. That's why I needed ALLOWED_COPY_ARGS from s3transfer.manager.TransferManager.

Technically your solution would work in the current moment, but that is just because we use only a single ExtraArgs function: Bucket.copy(). If in the future we introduce a second ExtraArgs function (there are several), then the new function will break without filtering. (Skipping the filtering here would build in a time-bomb.)

liormizr commented 3 years ago

OK I did some research. Now that I'm seeing the s3transfer is actually a part of boto3 (boto3 requirement and it's in the same boto organization) I have no issue using it.

Thank you @maresb , I learned something new :-)

liormizr commented 3 years ago

Looks great! I merged the PR

I'll update here with version number when I'll finish

liormizr commented 3 years ago

Deployed Version: 0.3.02