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 credentialProvider as an optional parameter of ConfigurationOptions #1339

Merged
merged 2 commits into from Feb 2, 2017
Merged

Add credentialProvider as an optional parameter of ConfigurationOptions #1339

merged 2 commits into from Feb 2, 2017

Conversation

RLovelett
Copy link
Contributor

As discussed in #1338, the documentation for AWS.S3 and AWS.SES both state that they accept credentialProvider, which is an instance of AWS.CredentialProviderChain, as part of the configuration options.

However, the declaration for ConfigurationOptions did not expose such a parameter. This patch resolves that issue.

Fixes #1338

As discussed in #1338 the documentation for AWS.S3 and AWS.SES both
state that they accept credentialProvider, which is an instance of
AWS.CredentialProviderChain, as part of the configuration options.

However, the declaration for ConfigurationOptions did not expose such a
parameter. This patch resolves that issue.

Fixes #1338
};
var s3 = new AWS.S3(options);
var ses = new AWS.SES(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisradek I am not exactly sure what the extent of the expected test should be. I feel like this covers the intent of what the change now provides. Obviously, if this is wrong I'll need a little bit more guidance on what to do next.

Copy link
Contributor

Choose a reason for hiding this comment

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

@RLovelett
This should be fine. We really just want to make sure the code compiles correctly, and that we don't break it in the future. This captures the intent.

@coveralls
Copy link

coveralls commented Feb 2, 2017

Coverage Status

Coverage remained the same at 91.863% when pulling 159a768 on RLovelett:bugfix/issue-1338 into 67f8e30 on aws:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.863% when pulling 159a768 on RLovelett:bugfix/issue-1338 into 67f8e30 on aws:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.863% when pulling 159a768 on RLovelett:bugfix/issue-1338 into 67f8e30 on aws:master.

@coveralls
Copy link

coveralls commented Feb 2, 2017

Coverage Status

Coverage remained the same at 91.863% when pulling 159a768 on RLovelett:bugfix/issue-1338 into 67f8e30 on aws:master.

Copy link
Contributor

@chrisradek chrisradek left a comment

Choose a reason for hiding this comment

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

Can you also run npm run add-change.

I think this could be classified as a bugfix with TypeScript as the category.

};
var s3 = new AWS.S3(options);
var ses = new AWS.SES(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

@RLovelett
This should be fine. We really just want to make sure the code compiles correctly, and that we don't break it in the future. This captures the intent.

@coveralls
Copy link

coveralls commented Feb 2, 2017

Coverage Status

Coverage remained the same at 91.863% when pulling 6c761de on RLovelett:bugfix/issue-1338 into 67f8e30 on aws:master.

@chrisradek chrisradek merged commit 0642da0 into aws:master Feb 2, 2017
@chrisradek
Copy link
Contributor

@RLovelett
Thanks for the PR! Merging!

@lock
Copy link

lock bot commented Sep 28, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TypeScript] S3.Types.ClientConfiguration and SES.Types.ClientConfiguration missing credentialProvider
3 participants