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: Move check for serviceUrl to createRequest #47

Merged
merged 1 commit into from
Sep 19, 2019

Conversation

mkistler
Copy link
Contributor

This PR moves the check for a non-empty serviceUrl to the createRequest method, so that sendRequest can be used from the authenticators which just pass url.

In addition, createRequest will take serviceUrl from the service baseOptions if none was provided in parameters.defaultOptions.

Tests were updated accordingly.

@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #47 into release-v1-rc2 will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release-v1-rc2      #47      +/-   ##
==================================================
+ Coverage           95.82%   95.95%   +0.13%     
==================================================
  Files                  23       23              
  Lines                 575      594      +19     
  Branches              121      133      +12     
==================================================
+ Hits                  551      570      +19     
  Misses                 23       23              
  Partials                1        1
Impacted Files Coverage Δ
lib/requestwrapper.ts 84.37% <ø> (-0.25%) ⬇️
lib/base_service.ts 100% <100%> (ø) ⬆️
lib/helper.ts 100% <0%> (ø) ⬆️
auth/utils/read-credentials-file.ts 93.1% <0%> (ø) ⬆️

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 1214b3d...aef7bd1. Read the comment docs.

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@mkistler
Copy link
Contributor Author

On further consideration, I decided to drop the logic that grabs the serviceUrl from baseOptions if one is not specified. That was a half solution that isn't needed here and should be addressed fully in a separate PR (in the future).

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@mkistler mkistler merged commit a71b087 into release-v1-rc2 Sep 19, 2019
@mkistler mkistler deleted the mdk/fix-serviceUrl-check branch September 19, 2019 14:08
ibm-devx-automation pushed a commit that referenced this pull request Oct 3, 2019
# [1.0.0](v0.3.6...v1.0.0) (2019-10-03)

### Bug Fixes

* Move check for serviceUrl to createRequest ([#47](#47)) ([6f04739](6f04739))
* parse result from response in token managers ([6bbe423](6bbe423))
* provide bundlers alternate file for browser support ([#58](#58)) ([88a9d16](88a9d16))

### Build System

* drop support for Node versions 6 and 8 ([#33](#33)) ([d47c737](d47c737))

### Code Refactoring

* look for credentials file in working dir before home dir ([#46](#46)) ([c5556de](c5556de))
* return detailed response as second callback argument ([#34](#34)) ([dc24154](dc24154))

### Features

* add `setServiceUrl` method as a setter for the `serviceUrl` property ([#41](#41)) ([cfb188f](cfb188f))
* add specific error handling for SSL errors with cloud private instances ([#54](#54)) ([056ec9a](056ec9a))
* export `UserOptions` interface from the BaseService ([#50](#50)) ([4f0075a](4f0075a))
* implement new authenticators to handle sdk authentication ([#37](#37)) ([f876b6d](f876b6d))
* refactor core to use Promises instead of callbacks ([#55](#55)) ([9ec8afd](9ec8afd))

### BREAKING CHANGES

* None of the authenticators or request methods take callbacks as arguments anymore - they return Promises instead.
* Users that have credential files in both the working directory and the home directory will see a change in which one is used.
* The internal property `url` no longer exists on the `baseOptions` object, it has been renamed to `serviceUrl`
* The old style of passing credentials to the base service will no longer work. An Authenticator instance MUST be passed in to the base service constructor.
* token managers no longer support user access tokens. use BearerTokenAuthenticator instead
* The class names of the token managers have changed.
* `Icp4dTokenManagerV1` renamed to `Cp4dTokenManager`
* `IamTokenManagerV1` renamed to `IamTokenManager`
* `JwtTokenManagerV1` renamed to `JwtTokenManager`
* The public method `setAuthorizationInfo` is renamed to `setClientIdAndSecret`
* The response body is no longer the 2nd callback argument, the detailed response is. The body is located under the `result` property. The `data` property is removed.
* This SDK may no longer work with applications running on Node 6 or 8.
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.

2 participants