-
Notifications
You must be signed in to change notification settings - Fork 48
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: add support for signed resumable upload URLs #290
Conversation
I'm sure my code is not production-worthy yet but I wanted to create the PR to help show what the problem is from my debugging. |
I signed the CLA. |
This doesn't handle v2 signed urls because they use a different query param ("Signature" I believe). But I'm not sure that's a good reason to not support V4 signed urls. We can add this but I don't have a test for it. |
@williambrode Thanks for the patch! Can you double-check that the e-mail you used to create the commit ( |
ed3c15e
to
1047680
Compare
1047680
to
30176d9
Compare
@tseaver You are right my e-mail was wrong in the commit. I've fixed 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.
This change looks reasonable, given the bug report, but we need to add unit tests to cover the new cases ("x-goog-signature" in parsed_query
/ ``"X-Goog-sSgnature" in parsed_query`).
@williambrode just wanted to follow up with you; are you able to write unit tests for this or would you like some support from us on that aspect. I know our testing framework might be a bit of a learning curve if this is your first external contribution :) |
Ah, I didn't quite realize that was directed at me. I can take a look at adding the test. |
Haha, reading over Tres' message again, it might not have been, I think I was hasty in my pass-through. |
Alright, gave a go at the unit test. How's that look @tseaver ? |
Updated the unit tests to cover signed url uploads. @tseaver PTAL, thanks! |
Fixes #289.