Skip to content
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 AWS Signature Version 4 #2036

Merged
merged 5 commits into from Jan 27, 2016
Merged

Add AWS Signature Version 4 #2036

merged 5 commits into from Jan 27, 2016

Conversation

simov
Copy link
Member

@simov simov commented Jan 26, 2016

mirkods and others added 4 commits January 26, 2016 11:34
added aws-sign4 support

added tests

Added sign_version parameter doc

fixed test

fix changes

removed changes on CHANGELOG

fix

fix

fix comma
@mirkods
Copy link
Contributor

mirkods commented Jan 26, 2016

When this will be merged?

@simov
Copy link
Member Author

simov commented Jan 26, 2016

I'm thinking about moving the require statement down below inside the aws handler and make aws4 external dependency. I don't feel comfortable adding more dependencies in request. Similar to how it's done in request/core.

@mirkods
Copy link
Contributor

mirkods commented Jan 27, 2016

Ok feel free to apply your edits, you know what are a good stuff for the module

simov added a commit that referenced this pull request Jan 27, 2016
@simov simov merged commit d9906af into request:master Jan 27, 2016
@simov
Copy link
Member Author

simov commented Jan 27, 2016

2.68 is published 🎉

, amazonHeaders: aws.canonicalizeHeaders(self.headers)

if (opts.sign_version == 4 || opts.sign_version == '4') {
var aws4 = require('aws4')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency was only added as a devDependency. See #2040

rmg added a commit to rmg/request that referenced this pull request Jan 27, 2016
It was moved in request#2036 as a devDependency, but is still used in
request.js

This should fix request#2040.
@rmg
Copy link
Contributor

rmg commented Jan 27, 2016

Sorry for the spam from commenting in multiple places, but this was a breaking change released as a patch. See #2040 and #2041.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants