-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Get Endpoint for Image metadata and presigned url #69
Add Get Endpoint for Image metadata and presigned url #69
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #69 +/- ##
===========================================
+ Coverage 98.09% 98.22% +0.13%
===========================================
Files 21 21
Lines 368 395 +27
===========================================
+ Hits 361 388 +27
Misses 7 7 ☔ View full report in Codecov by Sentry. |
One of the requirements for this issue listed here #37 (comment) , requires that the endpoint:
When given an object key that doesn't exist, the presigned url is still generated without any error being thrown. The issue only arises when you open the url and are greeted with this response. I wanted to check if this would still be a requirement for the back-end to handle or the front-end? S3 won't throw any exceptions when generating the url, so if the check has to be done in the back-end, it would require an additional call to S3 to get the file, implemeting something like this https://stackoverflow.com/questions/33842944/check-if-a-key-exists-in-a-bucket-in-s3-using-boto3 A similar questions was asked here https://stackoverflow.com/questions/78010875/how-to-directly-generate-presigned-url-if-key-exist-in-s3-or-upload-obj-and-cre, and the response was to add an explicit check referenced in my 1st link |
Yeah this behaviour is fine. I just checked the front end and it displays the right message when it fails to load from the URL so I it's fine to leave the URL in and ignore the check. |
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.
Some initial comments, one I am not sure on but we can discuss, worked well on the front end when we tested it.
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.
Just some minor comments and suggestions.
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.
Looks good, final few minor comments. Will get Viktor to have a look over the error handling next week to see what he thinks.
8a07c40
to
72374c9
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.
Minor docstring changes?
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.
Some more minor docstring changes
Description
See #37.
Testing instructions
Agile board tracking
closes #37