-
Notifications
You must be signed in to change notification settings - Fork 27
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
✨Storage can return a S3 link instead of a presigned link on demand #3004
✨Storage can return a S3 link instead of a presigned link on demand #3004
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
======================================
Coverage 79.6% 79.7%
======================================
Files 688 688
Lines 28735 28748 +13
Branches 3705 3707 +2
======================================
+ Hits 22899 22922 +23
+ Misses 5026 5015 -11
- Partials 810 811 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
6ffe337
to
bc50556
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Some observations and questions below.
@@ -343,7 +347,11 @@ async def upload_file(request: web.Request): | |||
source_uuid=source_uuid, | |||
) | |||
else: | |||
link = await dsm.upload_link(user_id=user_id, file_uuid=file_uuid) | |||
link = await dsm.upload_link( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsservations:
- if you look above there is this
extra_source
andextra_location
which ware optional. You should raise an error iflink_type
is also present. - If you select dactore/pennsieve as a backend, the s3 links should probably be disabled both in upload and in download. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I will check that in the next PR as I anyway need to bring that s3 link also to the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks!
What do these changes do?
This PR let storage return a valid S3 link for upload and download links.
This is a pre-requisite to be able to avail the 5GiB issue in AWS.
Modifies storage micro-service REST API like so:
s3
orpresigned
(default) when calling GET/PUT entrypoints for download/upload link respectivelyRelated issue/s
How to test
Checklist