Change order of credential path and read_external_sources and more #28

ehdsouza · 2019-09-16T19:25:50Z

This PR does the following:

Client properties like disable_ssl and url were read only from env, so like rest of the sdk's separated a read_external_sources
Re-ordeing of fetching credentials from working directory before home dir
Also removed setting of client properties in the constructor as they now go in the generated service classes

related to https://github.ibm.com/Watson/developer-experience/issues/7521

… dir

codecov · 2019-09-16T19:26:07Z

Codecov Report

Merging #28 into next will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next      #28      +/-   ##
==========================================
- Coverage   98.18%   98.15%   -0.04%     
==========================================
  Files          16       16              
  Lines         497      488       -9     
==========================================
- Hits          488      479       -9     
  Misses          9        9

Impacted Files	Coverage Δ
ibm_cloud_sdk_core/base_service.py	`97.39% <100%> (-0.15%)`	⬇️
ibm_cloud_sdk_core/utils.py	`97.97% <100%> (-0.05%)`	⬇️
ibm_cloud_sdk_core/__init__.py	`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 7ecc329...164d02e. Read the comment docs.

padamstx

Left a few comments that will need changes...

padamstx · 2019-09-16T20:06:14Z

ibm_cloud_sdk_core/utils.py

+    authenticator = None
+    config = read_external_sources(service_name)
+    if config:
+        authenticator = contruct_authenticator(config)


s/contruct/construct/

Also, "construct_authenticator" could probably be private since it should only be called from inside the get_authenticator_from_environment public method.

padamstx · 2019-09-16T20:09:40Z

ibm_cloud_sdk_core/base_service.py

@@ -65,7 +65,7 @@ def __init__(self,

        if display_name:
            service_name = display_name.replace(' ', '_').lower()


I think we also need to replace '-' with '_' in the service_name value. Some values for the vcap services name have -'s in them.

padamstx · 2019-09-16T20:11:49Z

ibm_cloud_sdk_core/utils.py

@@ -100,16 +111,16 @@ def read_from_credential_file(service_name, separator='='):
    # File path specified by an env variable
    credential_file_path = getenv('IBM_CREDENTIALS_FILE')

-    # Home directory
+    # Top-level of the project directory


This is actually the "Current working directory", right?

ibm_cloud_sdk_core/utils.py

padamstx · 2019-09-16T20:22:34Z

ibm_cloud_sdk_core/utils.py

@@ -125,15 +136,15 @@ def read_from_credential_file(service_name, separator='='):
    return config

 def _parse_key_and_update_config(config, service_name, key, value):
-    if service_name in key:
+    if key.startswith(service_name):
        index = key.find('_')


I don't think this works if the service_name contains an "_" does it???

You can see the logic that's used by the java core in CredentialUtils.parseCredentials(), to parse a line from either a credential file or an env var setting, which looks like this:

serviceName = serviceName.toUpperCase().replaceAll("-", "_") + "_"; ... if (key.startsWith(serviceName)) { String credentialName = key.substring(serviceName.length()); ... }

This will work as before calling the _parse_key_and_update_config , the calling function perform's the replace

The service_name passed into _parse_key_and_update_config could have an "_" in it, so this code fragment won't work:

if key.startswith(service_name): index = key.find('_') if index != -1: config[key[index + 1:]] = value

If this code is processing a key of "personality_insights_url", you'd end up with a config map with this entry:

"insights_url" --> <url value>

Right?

Please add a test that includes a multi-word service name and you will find out exactly what happens.

Ahaaa I see that the truncation happening here! Good catch! 👍

padamstx

More changes needed.

padamstx · 2019-09-17T20:12:17Z

ibm_cloud_sdk_core/utils.py

@@ -125,15 +136,15 @@ def read_from_credential_file(service_name, separator='='):
    return config

 def _parse_key_and_update_config(config, service_name, key, value):
-    if service_name in key:
+    if key.startswith(service_name):
        index = key.find('_')


The service_name passed into _parse_key_and_update_config could have an "_" in it, so this code fragment won't work:

if key.startswith(service_name): index = key.find('_') if index != -1: config[key[index + 1:]] = value

If this code is processing a key of "personality_insights_url", you'd end up with a config map with this entry:

"insights_url" --> <url value>

Right?

Please add a test that includes a multi-word service name and you will find out exactly what happens.

padamstx · 2019-09-17T20:13:37Z

resources/ibm-credentials-iam.env

+WATSON_AUTH_TYPE=iam
+WATSON_URL=https://gateway-s.watsonplatform.net/watson/api
+WATSON_DISABLE_SSL=False


Add some test properties that include a multi-word service name, rather than just "WATSON".

Erika can you add these tests properties?

ehdsouza · 2019-09-18T16:49:00Z

@padamstx addressed the multi word service name issue.

…ndividual services

ehdsouza · 2019-09-18T21:14:10Z

ibm_cloud_sdk_core/base_service.py

-        if display_name:
-            service_name = display_name.replace(' ', '_').lower()
-            config = read_from_env_variables(service_name)
-            if config.get('url'):


setting of client properties go in the generated service classes

padamstx

Getting closer, but more changes needed

padamstx · 2019-09-18T20:58:14Z

ibm_cloud_sdk_core/utils.py

-    if service_name in key:
-        index = key.find('_')
+    service_name = service_name.replace(' ', '_').replace('-', '_').lower()
+    key = key.replace(' ', '_').replace('-', '_').lower()


You shouldn't need to replace any characters in key as that is either a cred file property or an environment variable name.
Also, instead of folding service_name to lowercase, you could fold it to upper case when using it to parse property names from a credential file or environment variables and then just leave key alone. The credential file properties and environment variables should already be in upper case.

But for consistency isn't it better to have the key and service_name both converted?

What if a user has a environment variable like IBM-WATSON_USERNAME?

If the user doesn't follow the conventions, there's not much we can do about that. My opinion is that we shouldn't be changing the configuration data (cred file properties/env variables/VCAP_SERVICES) that the user has defined.

padamstx · 2019-09-18T21:10:04Z

ibm_cloud_sdk_core/utils.py

+    service_name = service_name.replace(' ', '_').replace('-', '_').lower()
+    key = key.replace(' ', '_').replace('-', '_').lower()
+    if key.startswith(service_name):
+        index = key.find(service_name)


since we entered this block because key starts with service_name, you shouldn't need to do a find at all, right?
since the key starts with the service_name the index should always be 0 (or 1, whatever index the first character has).
Please see the logic in the CredentialUtils.parseCredentials() function:
https://github.com/IBM/java-sdk-core/blob/e3d5330b0d9e2ef9cf084cb7c6aa97d6ae4f1fb4/src/main/java/com/ibm/cloud/sdk/core/util/CredentialUtils.java#L352
Specifically this:

serviceName = serviceName.toUpperCase().replaceAll("-", "_") + "_"; ... if (key.startsWith(serviceName)) { String credentialName = key.substring(serviceName.length());

Note that this function changes the serviceName value to do the character replacement AND adds a "_" on the end so that the updated value of serviceName (e.g. PERSONALITY_INSIGHTS_) will serve as the actual "prefix" that we'd expect to see in property and env variable names. Because we add the trailing "_" on the end, there's no +1 needed when we compute the length of serviceName.

Agree that I don't need to use find to get index, but how can we guarantee that the key would always be uppercase? I would prefer we transform both so that we perform the correct string matching

padamstx · 2019-09-18T21:14:30Z

ibm_cloud_sdk_core/utils.py


 def read_from_vcap_services(service_name):
-    service_name = service_name.replace(' ', '_').lower()
+    service_name = service_name.replace(' ', '_').replace('-', '_').lower()


When we get config info from VCAP_SERVICES, we should not do any transformations on service_name at all.
This is because the exact service name is configured in the API definition and is inserted into the generated code and should be used as-is. So, a vcap services name obtained from the API definition of personality-insights is the exact string that we'd expect to find within VCAP_SERVICES.

I perform the key conversion here: https://github.com/IBM/python-sdk-core/pull/28/files#diff-7048c45d573bb78c4bac9b0291b4e065R152 so that when service_name = personality-insights and vcap name is personality-insights would both be converted to personality_insights

Why? All you need to do is compare the "as-is" service_name as it was generated in the service and compare it with the outer-most map key within the VCAP_SERVICES json object. By trying to do all these extra, unnecessary transformations, you're actually accepting a different "language" than the other cores.

padamstx · 2019-09-18T22:30:10Z

ibm_cloud_sdk_core/utils.py

-    if service_name in key:
-        index = key.find('_')
+    service_name = service_name.replace(' ', '_').replace('-', '_').lower()
+    key = key.replace(' ', '_').replace('-', '_').lower()


If the user doesn't follow the conventions, there's not much we can do about that. My opinion is that we shouldn't be changing the configuration data (cred file properties/env variables/VCAP_SERVICES) that the user has defined.

padamstx · 2019-09-18T22:38:04Z

ibm_cloud_sdk_core/utils.py


 def read_from_vcap_services(service_name):
-    service_name = service_name.replace(' ', '_').lower()
+    service_name = service_name.replace(' ', '_').replace('-', '_').lower()


Why? All you need to do is compare the "as-is" service_name as it was generated in the service and compare it with the outer-most map key within the VCAP_SERVICES json object. By trying to do all these extra, unnecessary transformations, you're actually accepting a different "language" than the other cores.

padamstx

Looks good. I left one comment as a potential to-do item for when we implement the service config feature. One other future to-do item might be add a test involving VCAP_SERVICES for the "ibm-watson" service, but that can wait until we implement the service config feature I think.

padamstx · 2019-09-19T14:12:31Z

ibm_cloud_sdk_core/utils.py

-def contruct_authenticator(config):
-    auth_type = config.get('auth_type').lower() if config.get('auth_type') else 'iam'
+def _construct_authenticator(config):
+    auth_type = config.get('AUTH_TYPE').lower() if config.get('AUTH_TYPE') else 'iam'


I didn't comment on this before, and I don't think we need to change anything right now, but... when we implement the service config feature, it might be good to define constants for the various config properties rather than use the literal strings like this.

watson-github-bot · 2019-10-03T16:31:43Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

ehdsouza added 2 commits September 16, 2019 12:17

feat(base): Add method read_external_sources

8fee5bf

fix(credential): Check for credential file in working dir before home…

5c3d545

… dir

ehdsouza added the ready for review label Sep 16, 2019

ehdsouza requested review from mediumTaj and padamstx September 16, 2019 19:25

padamstx requested changes Sep 16, 2019

View reviewed changes

ehdsouza added 2 commits September 17, 2019 12:43

chore(replace): service name replace - to _

01912cd

chore(doc): Update comment

fe36514

ehdsouza requested a review from padamstx September 17, 2019 19:45

padamstx requested changes Sep 17, 2019

View reviewed changes

fix(config): Handle multi word service name

50fad59

ehdsouza added 3 commits September 18, 2019 13:50

fix(client props): Setting of the external client config happens in i…

4148034

…ndividual services

chore(service_url): Make service url as None

7981d92

test(service name): Mullti word service name

41a3a51

ehdsouza commented Sep 18, 2019

View reviewed changes

Update version.py

d21243c

padamstx requested changes Sep 18, 2019

View reviewed changes

ehdsouza added 3 commits September 18, 2019 14:38

chore(external_sources): Export externall sources

a3b5305

Merge branch 'order' of github.com:IBM/python-sdk-core into order

09fa9b5

chore(index): remove additional index checking

b7b3723

padamstx requested changes Sep 18, 2019

View reviewed changes

ehdsouza added 3 commits September 18, 2019 15:38

chore(key): can't replace key but only convert to lower

c4ba31d

fix(displlay_name): display_name unused in constructor

4742b8e

chore(upper): Use upper() instead of lower()

164d02e

ehdsouza changed the title ~~Change order of credential path and read_external_sources~~ Change order of credential path and read_external_sources and more Sep 19, 2019

padamstx approved these changes Sep 19, 2019

View reviewed changes

ehdsouza merged commit a304b1a into next Sep 19, 2019

ehdsouza deleted the order branch September 19, 2019 16:24

watson-github-bot added the released label Oct 3, 2019

		@@ -65,7 +65,7 @@ def __init__(self,

		if display_name:
		service_name = display_name.replace(' ', '_').lower()

Change order of credential path and read_external_sources and more #28

Change order of credential path and read_external_sources and more #28

Uh oh!

Conversation

ehdsouza commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehdsouza commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watson-github-bot commented Oct 3, 2019

Uh oh!

Uh oh!

ehdsouza commented Sep 16, 2019 •

edited

Loading

codecov bot commented Sep 16, 2019 •

edited

Loading