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.

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

✨(backends) add support for the AWS S3 backend #213

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

wilbrdt
Copy link
Contributor

@wilbrdt wilbrdt commented Sep 13, 2022

Purpose

Following issue #75, this PR aims to implement a new s3 backend using the low-level interface "client" from the boto3 library.

Proposal

  • add backend
  • add list functionality in backend
  • add fetch functionality in backend
  • add push functionality in backend
  • add tests for s3 backend
  • add backend documentation

Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, thanks! 🤩

I would recommend moving your commit title to:

✨(backends) add support for AWS S3

Note that the emoji character should be inserted as the first character of your commit title (and not :sparkle:).

setup.cfg Outdated Show resolved Hide resolved
src/ralph/backends/storage/s3.py Show resolved Hide resolved
src/ralph/backends/storage/s3.py Outdated Show resolved Hide resolved
src/ralph/backends/storage/s3.py Outdated Show resolved Hide resolved
src/ralph/backends/storage/s3.py Show resolved Hide resolved
Comment on lines 64 to 68
# Fix for incompatibility between moto and pyfakefs
# See https://github.com/spulec/moto/issues/1682
for module in [boto3, botocore]:
module_dir = Path(module.__file__).parent
fs.add_real_directory(module_dir, lazy_read=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried to factor this in the test suite setup() function?

Copy link
Contributor Author

@wilbrdt wilbrdt Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fs pyfakefs plugin from pytest has a function scope. Best I can do is factoring the code in a function scope fixture, but the fixture still needs to be called for each test method where moto and fs are used together. Pytest has fs_module or fs_session with larger scopes, but you can only choose one of the three, so it would have an impact on many tests.

@wilbrdt wilbrdt changed the title ✨(backends) add support for the AWS S3 backend ✨(backends) add support for the AWS S3 backend Sep 16, 2022
Copy link
Member

@quitterie-lcs quitterie-lcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Impressive job! 😍
Only a few remarks
Not simple at first sight to understand the test logic, but I really love it and find it very efficient

tests/backends/storage/test_s3.py Outdated Show resolved Hide resolved
tests/backends/storage/test_s3.py Show resolved Hide resolved
tests/backends/storage/test_s3.py Outdated Show resolved Hide resolved
tests/backends/storage/test_s3.py Outdated Show resolved Hide resolved
@wilbrdt wilbrdt force-pushed the add-aws-s3-backend branch 2 times, most recently from e7b5961 to 9594c70 Compare December 8, 2022 18:03
@wilbrdt wilbrdt force-pushed the add-aws-s3-backend branch 2 times, most recently from 17d392c to 6139c13 Compare December 9, 2022 16:32
Copy link
Member

@quitterie-lcs quitterie-lcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

CHANGELOG.md Outdated Show resolved Hide resolved
docs/backends.md Outdated Show resolved Hide resolved
tests/backends/storage/test_s3.py Outdated Show resolved Hide resolved
src/ralph/api/auth.py Outdated Show resolved Hide resolved
src/ralph/backends/storage/s3.py Show resolved Hide resolved
tests/backends/storage/test_s3.py Outdated Show resolved Hide resolved
@wilbrdt wilbrdt force-pushed the add-aws-s3-backend branch 5 times, most recently from 4e5f32d to e7fe2fb Compare December 14, 2022 11:00
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments. Almost good to go 💪

CHANGELOG.md Outdated Show resolved Hide resolved
docs/backends.md Outdated Show resolved Hide resolved
src/ralph/api/auth.py Outdated Show resolved Hide resolved
src/ralph/backends/storage/s3.py Show resolved Hide resolved
tests/fixtures/backends.py Outdated Show resolved Hide resolved
@wilbrdt wilbrdt force-pushed the add-aws-s3-backend branch 2 times, most recently from 77e7ad5 to e4002b0 Compare December 16, 2022 10:15
@wilbrdt wilbrdt force-pushed the add-aws-s3-backend branch 5 times, most recently from 9671b22 to 6a0f0f7 Compare December 26, 2022 16:33
Implement s3 backend using low-level interface client from the boto3 library
Fix pydocstyle linter now processing all src files and fix linter warnings.
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilbrdt wilbrdt merged commit 70be023 into openfun:master Jan 5, 2023
@wilbrdt wilbrdt deleted the add-aws-s3-backend branch January 5, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants