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

fix: allow iam client id and secret to be read from constructor #17

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

dpopp07
Copy link
Member

@dpopp07 dpopp07 commented Apr 25, 2019

@padamstx added support for IAM Client ID and Secret credentials in the Token Manager, but I realized that users wouldn't be able to pass these credentials into service constructors without additions to the Base Service. This PR adds those changes w/ tests.

@dpopp07 dpopp07 requested a review from esminerva April 25, 2019 16:32
@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #17 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #17      +/-   ##
=========================================
+ Coverage   95.54%   95.6%   +0.05%     
=========================================
  Files           7       7              
  Lines         337     341       +4     
  Branches       55      56       +1     
=========================================
+ Hits          322     326       +4     
  Misses         13      13              
  Partials        2       2
Impacted Files Coverage Δ
lib/base_service.ts 87.87% <ø> (ø) ⬆️
iam-token-manager/v1.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e46cbcf...875e8ce. Read the comment docs.

@padamstx
Copy link
Member

I noticed the same thing with Java and was about to get started on the Node-related changes to make it settable by the SDK user :)

@esminerva
Copy link
Contributor

We should add a check in token manager to check if secret is provided with client_id

@dpopp07
Copy link
Member Author

dpopp07 commented Apr 25, 2019

@joyychang We use the defaults if the user passes in only Client ID or Secret. So, the request would not actually fail but it won't do what the user might expect. I added a warning to let the user know that this is the case.

@padamstx I didn't catch this before but two of the test titles involved "passing only secret via constructor" - I think one was supposed to be using the setter so I changed that. Let me know if that wasn't the intention

@dpopp07 dpopp07 merged commit 3c88edb into master Apr 29, 2019
@dpopp07 dpopp07 deleted the read-iam-basicauth branch April 29, 2019 15:31
dpopp07 pushed a commit that referenced this pull request Apr 29, 2019
## [0.2.1](v0.2.0...v0.2.1) (2019-04-29)

### Bug Fixes

* allow iam client id and secret to be read from constructor ([#17](#17)) ([3c88edb](3c88edb))
@dpopp07
Copy link
Member Author

dpopp07 commented Apr 29, 2019

🎉 This PR is included in version 0.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

JurajNyiri pushed a commit to JurajNyiri/node-sdk-core that referenced this pull request Aug 22, 2024
Generated SDK source code using:
- Generator version 3.12.2
- Specification version 0.0.27
- Automation (cloudant-sdks) version 70e999d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants