-
Notifications
You must be signed in to change notification settings - Fork 99
impl(storage): logic for signed urls #4141
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
impl(storage): logic for signed urls #4141
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4141 +/- ##
==========================================
+ Coverage 94.72% 94.78% +0.06%
==========================================
Files 184 184
Lines 6933 7002 +69
==========================================
+ Hits 6567 6637 +70
+ Misses 366 365 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
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.
You need to convert the keys to lowercase on insert to the maps and revisit this logic.
And note that we are using BTreeMaps as the data structure, so we get sorting for free. You should be suspicious of any additional sorting. It probably means the code could be simpler.
|
@dbolduc I managed to get rid of the confusing code around sorting and lowercasing headers. In the end the only part that needs to keep ordering and casing is for Query Params, so for headers we can lowercase on insert and keep sort of BTreeMap. |
dbolduc
left a comment
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.
This is much easier on the eyes. Thanks.
thanks for the help on getting this into a much better state. Just pushed some updates to address the final nits. |
Add core logic for generating signed URLs for GCS. Tested against GCS conformance tests.
Towards #3645