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.

Sign up for GitHub

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

Jump to bottom

Updates CognitoIdentityCredentials to accept AWS.config options #1317

Merged

chrisradek merged 2 commits into aws:master from chrisradek:region-cognito-credentials

Jan 20, 2017

Contributor

chrisradek commented Jan 20, 2017

Fixes #1295


          Adds clientConfig to CognitoIdentityCredentials to support passing in…

98e4694

… region and other config to the underlying CognitoIdentity service client

jeskew reviewed

View reviewed changes

Contributor

jeskew left a comment

LGTM; just one question about STS

lib/credentials/cognito_identity_credentials.js

+                    var cognitoConfig = AWS.util.merge({}, clientConfig);
+                    cognitoConfig.params = this.params;
+                    this.cognito = new CognitoIdentity(cognitoConfig);
+                  }
                   this.sts = this.sts || new STS();

Contributor

jeskew Jan 20, 2017

Should the client config be passed to STS, too?

Contributor Author

chrisradek Jan 20, 2017

I went back and forth on that. Since STS uses a global endpoint, it's fine if the region the user passes in isn't us-east-1.

I would probably also have to update WebIdentityCredentials to accept client config, since it calls STS as well.

Contributor

jeskew Jan 20, 2017

There are other configuration options that customers might want to control (e.g., timeouts, retries, http agents, etc.)

Contributor Author

chrisradek Jan 20, 2017

Right. Sorry, I wasn't clear in my response. I was initially worried that a different region would cause an issue with STS, but discovered that's not the case. I'll update the PR to pass config to STS (and likewise to WebIdentityCredentials as well).

coveralls commented Jan 20, 2017 •

edited

Loading

Coverage increased (+0.008%) to 91.906% when pulling 98e4694 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

2 similar comments

coveralls commented Jan 20, 2017

Coverage increased (+0.008%) to 91.906% when pulling 98e4694 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

coveralls commented Jan 20, 2017

Coverage increased (+0.008%) to 91.906% when pulling 98e4694 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.


          Passes client config settings to STS clients in credentials

16d9d08

coveralls commented Jan 20, 2017

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

5 similar comments

coveralls commented Jan 20, 2017

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

coveralls commented Jan 20, 2017

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

coveralls commented Jan 20, 2017

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

coveralls commented Jan 20, 2017

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

coveralls commented Jan 20, 2017 •

edited

Loading

Coverage increased (+0.01%) to 91.912% when pulling 16d9d08 on chrisradek:region-cognito-credentials into c40f0d6 on aws:master.

jeskew approved these changes

View reviewed changes

Contributor

jeskew left a comment

🚢 🇮🇹

chrisradek merged commit f9f49cb into aws:master

chrisradek deleted the region-cognito-credentials branch

January 20, 2017 23:46

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 bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet