inveniosoftware / invenio-s3

S3 file storage support for Invenio.
https://invenio-s3.readthedocs.io
MIT License
0 stars 16 forks source link

Enable connection to S3 API for Swift, and other minor fixes #8

Open borellim opened 4 years ago

borellim commented 4 years ago

Hello.

I made some changes to interface invenio-s3 with an S3-over-Swift compatibility API that we use. I also took the chance to fix a few misspellings. I hope this is the right way to do it, but if not I'm happy to discuss.

Changes:

borellim commented 4 years ago

In case you're wondering why I didn't leave an empty string as the default value of acl, it's because or object store complains that null is not a valid value for the ACL; apparently '' is converted into null somewhere. I couldn't find a better place to set this to 'private'.

Also I just realized that this partially overlaps with #6. Sorry...

borellim commented 4 years ago

test_delete() from tests/test_storage.py also fails on master for me, not sure why.

egabancho commented 4 years ago

Hi Marco, thanks for the contribution! I hope you don't mind that I've merged already some of the changes you did, https://github.com/inveniosoftware/invenio-s3/pull/10, they actually made sense and there was no room for discussion ☺️

Regarding the ACL, I am not completely sure how to go about this, I not very inclined towards hard-codding the ACL default value, specially since we already have a default value working for AWS and other S3 compatible installations. But, for sure we can come up with a solution that makes us both happy!

I am thinking, just one idea, about making a contrib folder containing the File Storage implementations with the peculiarities needed for the third party S3 compatible vendors, like Swift.

Imaging having one file there for Swift, with the class needed extending S3FSFileStorage, overwriting whatever you need, and with a storage factory, like this one https://github.com/inveniosoftware/invenio-s3/blob/master/invenio_s3/storage.py#L151 but using your File Storage class instead. Then it's just a matter of setting the correct factory depending on your backend. Like this we keep the "base" as implementation agnostic as possible and we provide you, and any other who comes after, the possibility to somehow extend it and contribute back this "peculiar" implementations.

What do you think? cc @inveniosoftware/architects

lnielsen commented 4 years ago

@egabancho I agree with the contrib package.

borellim commented 4 years ago

Hello. Thanks a lot for your reply. Really sorry about my delayed response.

I'm absolutely happy that you decided to merge some of my changes already. I spotted a couple that are still needed, so I left them in this rebased branch. Feel free to take again what you need, it's not a problem to rebase again.

As for the ACL options, I'll let you decide what's best. I would be fine with a contrib folder with specific implementations.

egabancho commented 4 years ago

As for the ACL options, I'll let you decide what's best. I would be fine with a contrib folder with specific implementations.

@borellim I think you can go ahead and move this into contrib/swift.py