feat: introduce v4 signed url #637
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #637 +/- ##
==========================================
+ Coverage 97.92% 98.04% +0.11%
==========================================
Files 9 9
Lines 869 921 +52
Branches 99 100 +1
==========================================
+ Hits 851 903 +52
Misses 9 9
Partials 9 9
Continue to review full report at Codecov.
|
|
@jkwlui thanks for hanging with me on the feedback on this one 🙃 |
jkwlui
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
Not at all LOL I appreciate the feedback! hold off merging until @frankyn had a chance to take a look :) |
| }); | ||
|
|
||
| let promise: Promise<SignedUrlQuery>; | ||
| if (version === 'v2') { |
There was a problem hiding this comment.
Could you log the future change:
You have generated a signed URL using the default v2 signing implementation. In the future, this will default to v4. You may experience breaking changes if you use longer than 7 day expiration times with v4. To opt-in to the behavior specify config.version='v4'
There was a problem hiding this comment.
We don't really do these in Node.
@JustinBeckwith do we have any best-practices/alternatives for warning users of impending change in API during runtime in addition to jsdoc notes?
There was a problem hiding this comment.
Yes we do! Checkout what I did here for google-auth-library:
https://github.com/googleapis/google-auth-library-nodejs/blob/master/src/messages.ts
You want to use process.emitWarning, but there is a lot of nuance. Following gal is probably a good idea.
There was a problem hiding this comment.
Sorry, google-auth-library
There was a problem hiding this comment.
Only default is fine in this case.
There was a problem hiding this comment.
Yes we do!
Heh I must've remembered a different conversion about emitting warnings in Node. 😛
I am trying to add a similar emitWarning logic here and I feel like I'm duplicating most of the code you have on google-auth-library. Any hints here, or should I just go ahead with it?
There was a problem hiding this comment.
It's entirely possible there's a library to do this - not sure! @googleapis/node-team?
There was a problem hiding this comment.
@JustinBeckwith are you okay with me keeping the (mostly duplicated) code? I've looked high and low and couldn't find anything :(
* Revert "split out conformance test" This reverts commit ce00255. * sync conformance-test cases; parse cases without modification * this.skip * refactor: lift interface
|
@jkwlui please merge and release when issues are resolved. |
requires googleapis/google-auth-library-nodejs#649 for system-tests to pass