Feature/s3 storage #86
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test cases depend on MiniO
An alembic migration has been introduced for it The LibraryRegistrar.register method uploads logos to S3 now
Thus removing 300+ deprecation warnings from the test result
Some other minor changes
…efault boto3 credential management
|
This looks ready to go to me. Only reason I'm not approving it right now is that I would like to test the migrations against our production database before we merge. Going to get that test setup and make sure everything goes smoothly, then will approve. |
One for unsigned get-object requests, and another for signed put-object requests It seems we cannot do both through a single client
jonathangreen
approved these changes
Dec 1, 2022
There was a problem hiding this comment.
Tested with a copy of the production database, and this worked well. I pushed a couple commits, that you should take a look at before merging @RishiDiwanTT and let me know if you have any feedback.
- Update
docker-compose.ymlto have minio and necessary env variables for testing - Updated the path of the stored logo to use the uuid instead of the ID. This is consistent with the other links we expose, the seem to use the uuid rather then the database ID of the item.
All looks good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Library registry now pushes the library logos it gets from the authentication documents to an S3 bucket
It also stores the URL for the logo in the
Library.logo_urlattributeThe logo file is stored with a
public-readACL, so no presigned urls are required, making it CDN friendlyThe storage class is written by abstracting away boto3 internals, so other platforms can be implemented when required.
Only basic operations have been implemented
Additionally,
Motivation and Context
The library-registry libraries feed did not contain the library logo urls, causing mobile apps to unnecessarily make additional requests to the CMs. This change is meant to reduce that traffic.
Notion
How Has This Been Tested?
Manually tested with a MiniO container, no S3 testing.
Unit tests have been written to encompass the change.
Checklist