feat: changes to node-sdk-core to work with service factory feature #72

SaltedCaramelCoffee · 2019-11-14T21:35:23Z

Checklist

npm test passes (tip: npm run lint-fix can correct most style issues)
tests are included
documentation is changed or added

CLAassistant · 2019-11-14T21:39:10Z

All committers have signed the CLA.

codecov · 2019-11-14T21:40:45Z

Codecov Report

Merging #72 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #72   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           8      8           
  Branches        1      1           
=====================================
  Hits            8      8

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 dbe1064...17503f9. Read the comment docs.

padamstx

Looks good... just left a few comments about some nitpicky things :)
I'll approve assuming those are addressed.
I suggested alternate comments for testcases and a new name for a function or two. My specific naming suggestions aren't that important, but the general point is to maybe just use a more descriptive name, etc.

padamstx · 2019-11-15T18:41:08Z

auth/utils/read-external-sources.ts

@@ -97,8 +96,31 @@ function filterPropertiesByServiceName(envObj: any, serviceName: string): any {
 * Pulls credentials from VCAP_SERVICES env property that IBM Cloud sets
 *
 */
+function getCredentials(name) {


After copying this code from its original package into this file, I think it would be good to give this a more descriptive name.
In fact, I'll suggest that you rename getCredentialsFromCloud to be getPropertiesFromVCAP and then rename getCredentials to be something like getVCAPCredentialsForService, or just move the getCredentials logic into the newly-renamed getPropertiesFromVCAP.

padamstx · 2019-11-15T18:46:34Z

test/unit/getCredentials.test.js

+    expect(credentials.password).toEqual("not-a-password")
+  });
+
+  it('should return first service in the list when multiple services found', () => {


Suggested change

it('should return first service in the list when multiple services found', () => {

it('should return first service in the list when matching on primary service key', () => {

padamstx · 2019-11-15T18:48:10Z

test/unit/getCredentials.test.js

+    expect(Object.keys(credentials).length).toEqual(0);
+  });
+
+  it('should return empty credential object when looking for credentials by service name', () => {


Suggested change

it('should return empty credential object when looking for credentials by service name', () => {

it('should return credential object matching service name field not first list element', () => {

padamstx · 2019-11-15T18:50:06Z

test/unit/getCredentials.test.js

+    expect(credentials.password).toEqual("not-a-password")
+  });
+
+  it('should return empty credential object when looking for credentials by service name', () => {


Suggested change

it('should return empty credential object when looking for credentials by service name', () => {

it('should return credential object when matching service name field on first list element', () => {

padamstx · 2019-11-15T18:51:38Z

test/unit/getCredentials.test.js

+    expect(credentials.password).toEqual("not-a-password")
+  });
+
+  it('should return empty credential object when service key is no-creds-service-two', () => {


Suggested change

it('should return empty credential object when service key is no-creds-service-two', () => {

it('should return empty credential object when matching on service name with no credentials field', () => {

…ureService

dpopp07

This looks good! I made a few suggestions, which don't necessarily need to be adhered to. I'm mostly requesting changes to get clarity on if this is a breaking change or not before we merge

dpopp07 · 2019-11-18T16:54:15Z

test/resources/vcap.json

+      "plan": "elite"
+    }
+  ]
+}


nit: will you add a newline here?

Suggested change

}

}

lib/base-service.ts

auth/utils/read-external-sources.ts

dpopp07 · 2019-11-18T17:11:18Z

auth/utils/read-external-sources.ts

-  const credentials = vcapServices.getCredentials(serviceName);
+function getVCAPCredentialsForService(name) {
+  if (process.env.VCAP_SERVICES) {
+    const services = JSON.parse(process.env.VCAP_SERVICES);


I think it would be safer to wrap the JSON parse in a try/catch since it throws an error if anything about the JSON is malformatted. Not that it should be, since that env variable is generated by the cloud. So this is an optional change, up to you.

Suggested change

const services = JSON.parse(process.env.VCAP_SERVICES);

let services;

try {

services = JSON.parse(process.env.VCAP_SERVICES);

} catch () {

return {};

}

Wrapping the JSON parse in a try/catch might eat up/silence the error, and the user wouldn't know their VCAP_SERVICES env is not parsed. So I think it's fine to let it fail if for some reason JSON is malformatted.

Okay, that's a fair point. I just remembered that the core has it's own logger now. So another thing to consider would be printing an error log for a malformatted VCAP variable so the user knows about it. My only concern here is the code crashing even though the user typically cannot control the contents of VCAP_SERVICES.

auth/utils/read-external-sources.ts

…ency

dpopp07 · 2019-11-18T19:32:21Z

auth/utils/read-external-sources.ts

+        if (services[serviceName].length > 0) {
+          return services[serviceName][0].credentials || {};
+        } else {
+          return {}


Speaking of the logger, I think it's a good idea to add debug or verbose logging statements before an empty object is returned saying something like "no data read from vcap services"

dpopp07 · 2019-11-18T21:35:07Z

auth/utils/read-external-sources.ts

+      }
+    }
+  }
+  return {};


One more place to add a debug

dpopp07 · 2019-11-18T21:37:03Z

auth/utils/read-external-sources.ts

+ *
+ * If no match against the service entry's "name" field is found, then find the
+ * service list with a key matching the serviceKey value. If found, return its
+ * credentials.


Should mention that you are returning the first credentials object in the service list.

dpopp07

Looks good! 👍

Make sure you format the commit message properly to ensure a good message in the CHANGELOG

# [2.0.0](v1.3.0...v2.0.0) (2019-11-19) ### Features * changes to node-sdk-core to work with service factory feature ([#72](#72)) ([cde4cd6](cde4cd6)) ### BREAKING CHANGES * The `BaseService` will no longer look for configurations externally by default. A new factory method is provided to create an instance from external configuration. * feat: changes to node-sdk-core to work with service factory feature * `BaseService` constructor will no longer call `configureService`. * updated test to reflect base service constructor does not call configureService * added test for getting credentials from vcap * removed `name` and `serviceVersion` because they are not referenced anymore * added comment for vcap parsing function. removed vcap_services dependency * added debug messages for when returning empty credential

ibm-devx-automation · 2019-11-19T17:13:05Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

SaltedCaramelCoffee requested a review from dpopp07 November 14, 2019 21:35

padamstx self-requested a review November 14, 2019 21:37

padamstx approved these changes Nov 15, 2019

View reviewed changes

SaltedCaramelCoffee force-pushed the feat-service-factory branch from 3fab5cd to cdf8729 Compare November 15, 2019 20:26

Shuyu Chen added 7 commits November 15, 2019 15:34

feat: changes to node-sdk-core to work with service factory feature

0576380

updated test to reflect base service constructor does not call config…

5d907b9

…ureService

fixed linter issues

f0030cb

added test for getting credentials from vcap

e770cc7

linted test

6d5bebb

changed function name and wrote better test descriptions

8f399ce

removed name and serviceVersion because they are not referenced anymore

c068ba7

SaltedCaramelCoffee force-pushed the feat-service-factory branch from cdf8729 to c068ba7 Compare November 15, 2019 21:35

changed import to import from build

afefb80

dpopp07 requested changes Nov 18, 2019

View reviewed changes

added comment for vcap parsing function. removed vcap_services depend…

940ecde

…ency

dpopp07 reviewed Nov 18, 2019

View reviewed changes

SaltedCaramelCoffee force-pushed the feat-service-factory branch from 55a1100 to 6caf329 Compare November 18, 2019 21:32

dpopp07 reviewed Nov 18, 2019

View reviewed changes

SaltedCaramelCoffee force-pushed the feat-service-factory branch from 6caf329 to d4e318d Compare November 18, 2019 21:56

added debug messages for when returning empty credential

17503f9

SaltedCaramelCoffee force-pushed the feat-service-factory branch from d4e318d to 17503f9 Compare November 18, 2019 23:17

dpopp07 approved these changes Nov 19, 2019

View reviewed changes

SaltedCaramelCoffee merged commit cde4cd6 into master Nov 19, 2019

ibm-devx-automation added the released label Nov 19, 2019

padamstx deleted the feat-service-factory branch November 10, 2020 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: changes to node-sdk-core to work with service factory feature #72

feat: changes to node-sdk-core to work with service factory feature #72

SaltedCaramelCoffee commented Nov 14, 2019

CLAassistant commented Nov 14, 2019 •

edited

Loading

codecov bot commented Nov 14, 2019 •

edited

Loading

padamstx left a comment

padamstx Nov 15, 2019

padamstx Nov 15, 2019

padamstx Nov 15, 2019

padamstx Nov 15, 2019

padamstx Nov 15, 2019

dpopp07 left a comment

dpopp07 Nov 18, 2019

dpopp07 Nov 18, 2019

SaltedCaramelCoffee Nov 18, 2019

dpopp07 Nov 18, 2019

dpopp07 Nov 18, 2019

dpopp07 Nov 18, 2019

dpopp07 Nov 18, 2019

dpopp07 left a comment

ibm-devx-automation commented Nov 19, 2019

	it('should return first service in the list when multiple services found', () => {
	it('should return first service in the list when matching on primary service key', () => {

	it('should return empty credential object when looking for credentials by service name', () => {
	it('should return credential object matching service name field not first list element', () => {

	it('should return empty credential object when looking for credentials by service name', () => {
	it('should return credential object when matching service name field on first list element', () => {

feat: changes to node-sdk-core to work with service factory feature #72

feat: changes to node-sdk-core to work with service factory feature #72

Conversation

SaltedCaramelCoffee commented Nov 14, 2019

Checklist

CLAassistant commented Nov 14, 2019 • edited Loading

codecov bot commented Nov 14, 2019 • edited Loading

Codecov Report

padamstx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Nov 19, 2019

CLAassistant commented Nov 14, 2019 •

edited

Loading

codecov bot commented Nov 14, 2019 •

edited

Loading