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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

borellim
Copy link
Contributor

@borellim borellim commented Oct 22, 2019 •
edited

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:

  • (merged) add configuration variable S3_SIGNATURE_VERSION (allows to use the older v2 signature protocol);
  • S3FSFileStorage:
    • in methods update() and initialize(), add parameter acl with default value 'private';
    • add new method open() that accepts a parameter 'acl' with default value 'private';
    • in all 3 methods above, pass the value of acl to S3FileSystem._open() via AbstractFileSystem.open();
  • (partially merged) docstring: turn suggested location name from S3_default to s3-default, as capital letters and underscores are not accepted by the invenio files location command;
  • (partially merged) fix misspellings in configuration variables:
    • S3_ACCCESS_KEY_ID -> S3_ACCESS_KEY_ID
    • S3_SECRECT_ACCESS_KEY -> S3_SECRET_ACCESS_KEY
  • (merged) fix misspelling in property init_s3f3_info -> init_s3fs_info

@borellim
Copy link
Contributor Author

borellim commented Oct 22, 2019 •
edited

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
Copy link
Contributor Author

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

@borellim borellim mentioned this pull request Oct 22, 2019
Open
@egabancho egabancho self-requested a review October 31, 2019 14:44
@egabancho egabancho self-assigned this Oct 31, 2019
@egabancho
Copy link
Member

Hi Marco, thanks for the contribution! I hope you don't mind that I've merged already some of the changes you did, #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
Copy link
Member

@egabancho I agree with the contrib package.

@borellim
Fix typos in config variable
e966a63 
S3_SECRECT_ACCESS_KEY -> S3_SECRET_ACCESS_KEY
@borellim
Add "acl" parameter to initialize() and update() of S3FSFileStorage;
44e721a 
also add a new method open() in S3FSFileStorage, that overrides
that of PyFSFileStorage, so that we can actually pass the ACL string
to `S3FileSystem._open()`.
@borellim
Minor fix in documentation
c0e9617 
Location names with underscores are not valid.
@borellim
Use 'private' as default acl
2a65a08
@borellim
Add config variable S3FS_BLOCK_SIZE
53ab55d 
This sets property default_block_size when creating an instance of
class S3FileSystem.
@borellim
Copy link
Contributor Author

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
Copy link
Member

egabancho commented Mar 1, 2020 •
edited

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

@egabancho egabancho removed their assignment Apr 29, 2020
@liotf liotf deleted the misc_small_improvements branch November 11, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egabancho egabancho Awaiting requested review from egabancho

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@borellim @egabancho @lnielsen