Update credential file path check to current working directory #50
Conversation
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 97.58% 97.75% +0.17%
==========================================
Files 17 17
Lines 579 579
==========================================
+ Hits 565 566 +1
+ Misses 14 13 -1
Continue to review full report at Codecov.
|
ibm_cloud_sdk_core/utils.py
Outdated
| from os import getenv, environ | ||
| from os.path import dirname, isfile, join, expanduser, abspath | ||
| from os import getenv, environ, getcwd | ||
| from os.path import isfile, join, expanduser, abspath |
There was a problem hiding this comment.
| from os.path import isfile, join, expanduser, abspath | |
| from os.path import isfile, join, expanduser |
This change will fix the linter error that is causing the build to fail.
There was a problem hiding this comment.
I removed the unused path and I changed a test to load the credentials file from the current working directory instead of the absolute path of ../../file but that doesn't test the exact reason this error happened. It happened because the core was being used one directly higher in the python sdk. This is fixed but I currently dont know an easy was of testing that scenario.
There was a problem hiding this comment.
Can you have a test that relies on a file in a specific directory X (e.g. X/ibm-credentials.env) where the test first performs the equivalent of the "cd X" shell command? Then when you trigger the loading of the credentials file, the one in the (now) "current" directory should be found.
There was a problem hiding this comment.
Good idea, I now modified two tests. For test_iam, I changed it to load the ibm-credentials.env file from the current working directory instead of from the file_path environment variable. For test_configure_services, I did what your comment suggested and changed working directories to the credentials file, then triggered loading the credentials file in the new current directory.
test/test_base_service.py
Outdated
| cwd = os.getcwd() | ||
| os.chdir(os.path.dirname(file_path)) | ||
| iam_authenticator = get_authenticator_from_environment('ibm-watson') | ||
| os.chdir(cwd) |
There was a problem hiding this comment.
I don't quite understand why you'd restore the original CWD immediately after loading up an authenticator but before you construct the service instance (which presumably would make a call to BaseService.configure_service(). What scenario are you trying to test exactly? What would make sense to me would be to "cd" to the directory containing the ibm-credentials.env file, then try to construct an instance of the service while passing in a service name of "ibm_watson". This should load up an authenticator via external config AND configure the service instance using the properties contained in that file.
There was a problem hiding this comment.
I mistakenly thought that getting the authenticator was the only thing that needed to be in the current working directory so I moved my cleanup statements to immediately after that so they would be called even if the assert statements failed. I moved it to it's correct location and it's working now, thanks!
test/test_base_service.py
Outdated
| temp_env_path = os.getcwd() + '/ibm-credentials.env' | ||
| copyfile(file_path, temp_env_path) | ||
| iam_authenticator = get_authenticator_from_environment('ibm-watson') | ||
| os.remove(temp_env_path) |
There was a problem hiding this comment.
This is the same as the first scenario, except in the case above you were "cd"'ing back to the original working directory (hence hiding the ibm-credentials.env file) and in this case you're actually deleting the file prior to constructing the service. In both cases, the file will not be "seen" when the service is being constructed.
There was a problem hiding this comment.
I think what we need to test (primarily) is a simple scenario like this:
- change the CWD to the location where the
ibm-credentials.envfile exists - load an authenticator using the factory and pass in "ibm_watson" for the service name; verify that it is the correct type of authenticator (based on the properties defined in
ibm-credentials.env) - construct an instance of the test service and call its
configure_service()method (passing in "ibm_watson" for the service name; verify that the service url field of the service is set to the URL specified inibm-credentials.env.
There was a problem hiding this comment.
Right, I mistakenly put my cleanup statements in the wrong place so the service was defaulting to the default URL. I fixed it as you said and it's working as expected now. Thanks!
|
🎉 This PR is included in version 1.5.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
It currently incorrectly checks in the core root directory.
Closes https://github.ibm.com/arf/planning-sdk-squad/issues/1253