Ability to specify prefix for S3 storage

[eprikazc]

This change adds S3PrefixedStorage class which extends S3Storage with "prefix" required argument.
I thought it would be nice to be ale to store all files within one folder in S3. So, I implemented it after reviewing this discussion:
https://github.com/amol-/depot/issues/13#issuecomment-142432557
As suggested, prefix here is set in storage, not in the file field.

Can you please review and provide feedback? One thing I am not sure about in this implementation is that prefix ends up in UploadedFile['file_id']. Perhaps it would be more clear to not leak prefix outside of the storage class.

[amol-]

Just a few questions that arose in my mind:

• Is there any specific reason for S3PrefixedStorage? Couldn't it be just an option of S3Storage that can be provided or not?
• Yep, I think that keeping the prefix only on the storage and not leaking it to the file_id would lead to a more coherent behaviour. It would behave as all the other storage options. Otherwise we have a special case where changing that option actually doesn't involve past files (as the prefix is stored with them). Which might lead people to think that also all the other options behave like that or vice versa.

[landscape-bot]

Code quality remained the same when pulling cc0d184 on eprikazc:master into 0af12ff on amol-:master.

[eprikazc]

Thanks for the feedback.
#1 - I thought that extending would be more clear approach than adding IF-statements here and there. This way code looks more readable.
#2 - OK, I will update it soon.

[rlam3]

@eprikazc how do i use this? Do you have documentation for this? Thanks!

I did something similar :

def __init__(... upload_directory=None...)

self.upload_directory = upload_directory or ''

into S3Storage instead to get this to work. When I configure my depot,I used the depot name as the upload directory ...

[eprikazc]

@rlam3 - see eprikazc@acc85ae
I introduced S3PrefixedStorage that subclasses S3Storage and requires one extra argument - "prefix"

[amol-]

That was one of the reasons why I was concerned about subclassing, it would be a bit confusing for users :D

Maybe we can expose a Factory to provide a single name while bulding the different classes? Not sure the improvement in usability justifies the effort of having the factory and maintaining it on long-term. Your opinion?

[rlam3]

@eprikazc So I'm assuming that you are doing something along the lines of

DepotManager.config('avatar',{
    ...
    'depot.prefix':'xyz',
   ...
}

Am I correct? If that is the case, why do you need to subclass? why not just add it right into S3Storage?

rlam3@4fcc262

[eprikazc]

@amol- I have found simpler way to implement prefix support - via wrapper around bucket (BucketDriver). So, now prefix can be passed as optional parameter to S3Storage. Please take a look.

@rlam3 - see above.

[amol-]

👍 far better approach.

I think that we can rename the bucket_driver as an internal variable (_bucket_driver), update the S3Storage docstring to document the prefix option and then we are ready to merge 😄

[eprikazc]

Done!

[rlam3]

Is this how I configure it?

DepotManager.configure(
    'photos',{
    'depot.backend':'depot.io.awss3.S3Storage',
    'depot.access_key_id':S3_KEY,
    'depot.secret_access_key':S3_SECRET,
    'depot.bucket':S3_BUCKET,
    'prefix' : 'photos',
    },
)

[eprikazc]

Yes. Though, I think prefix should be "photos/". Any problems with it?

[rlam3]

@eprikazc

Documentation show that it should be outside of the config dictionary... Could you please verify and correct this if it is wrong? Thanks!

http://depot.readthedocs.org/en/latest/api.html#depot.manager.DepotManager

[amol-]

You are confusing the configuration prefix with the S3Storage key prefix -> http://depot.readthedocs.org/en/latest/api.html#depot.io.awss3.S3Storage

Just pass a prefix key in the options of the DepotManager when using S3Storage to specifiy an S3 key prefix.

[rlam3]

@amol- Correct me if I'm wrong.

DepotManager prefix is used for the prefix of the configuration dictionary that is passed in.
Whereas, the 'depot.prefix' inside of the config dictionary for depot manager is the prefix for s3 folder to be created within the folder of the bucket.

Thanks

[amol-]

@rlam3 yes, that's it