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

feat: support arbitary boto3 s3 config options in s3 backend #1697

Merged
merged 6 commits into from
Apr 8, 2024

Conversation

joel-aws
Copy link
Contributor

@joel-aws joel-aws commented Apr 2, 2024

No description provided.

Copy link
Contributor

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Hi and thanks for this. I am sure it will be helpful for S3 bandersnatch users.

Is there any easy way we can use our minio unit/integration tests to make use of any of these config params to see at least one work? I feel this will help with future API changes etc. etc.

src/bandersnatch_storage_plugins/s3.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.59%. Comparing base (4d020e8) to head (c8ce794).
Report is 58 commits behind head on main.

Files Patch % Lines
src/bandersnatch_storage_plugins/s3.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1697      +/-   ##
==========================================
+ Coverage   79.69%   83.59%   +3.90%     
==========================================
  Files          31       31              
  Lines        4324     4335      +11     
  Branches      780      782       +2     
==========================================
+ Hits         3446     3624     +178     
+ Misses        721      524     -197     
- Partials      157      187      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joel-aws
Copy link
Contributor Author

joel-aws commented Apr 2, 2024

@cooperlees -- added a test that uses minio.

I also refactored s3.py to apply the boto3 configuration to all instantiations of S3Path. Though, when .s3keep objects are created, the configuration is not applied (couldn't figure that one out). Overall, I don't love my implementation, so I welcome suggestions.

Copy link
Contributor

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

I don't have suggestions no how to make this any cleaner. So I am happy to merge if you are.

Though, when .s3keep objects are created, the configuration is not applied

Is there any open issue or is it worth opening an issue here to see if there is a bug or we're doing something wrong here?

If you are happy here I'll merge and if it benefits you cut a new release with the feature.

Thanks for the contribution! (I am no s3 guru tho)

CHANGES.md Outdated Show resolved Hide resolved
backend = s3.S3Storage(config=config_loader.config)
backend.initialize_plugin()

assert backend.configuration_parameters["ServerSideEncryption"] == "AES256"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I like that we have this - Long as AES256 isn't default :D

@cooperlees
Copy link
Contributor

@joel-aws Happy for this to be merged?

@joel-aws
Copy link
Contributor Author

joel-aws commented Apr 8, 2024

@cooperlees LGTM!

@cooperlees cooperlees merged commit 5642501 into pypa:main Apr 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants