-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fixes issue wherein the wrong parameter name was used for cross region presigned urls #1325
Conversation
5 similar comments
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.
🐑 🇮🇹
var doesParamValidation = !!this.config.paramValidation; | ||
// remove the validate parameters listener so we can re-add it after we build the URL | ||
if (doesParamValidation) { | ||
request.removeListener('validate', AWS.EventListeners.Core.VALIDATE_PARAMETERS); |
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.
I wish we had a better way to reorder listeners instead of just removing and re-adding them. Changing this is out of scope for this CR, but it's something we should take a look at.
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.
Agreed, we do this often enough throughout the SDK that we should provide a better way to handle this.
@@ -215,7 +215,7 @@ describe 'AWS.EventListeners', -> | |||
makeRequest(->) | |||
expect(options.timeout).to.equal(15) | |||
|
|||
it 'signs only once in normal case', -> | |||
it 'signs only once in normal case', (done) -> |
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.
Hopefully, making all of these async should take care of the flickering failures we've been seeing over the past few weeks
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Fixes #1324
/cc @jeskew