feat(icp4d): Add support for icp4d #17
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
+ Coverage 95.7% 95.75% +0.05%
==========================================
Files 7 9 +2
Lines 396 448 +52
==========================================
+ Hits 379 429 +50
- Misses 17 19 +2
Continue to review full report at Codecov.
|
| URL = 'url' | ||
| USERNAME = 'username' | ||
| PASSWORD = 'password' | ||
| IAM_APIKEY = 'iam_apikey' | ||
| IAM_URL = 'iam_url' | ||
| ICP4D_URL = 'icp4d_url' |
There was a problem hiding this comment.
I'm curious about the reasoning behind giving ICP4D it's own URL. If I understand correctly, ICP4D has one URL and the tokens are retrieved from a certain endpoint on the same cluster as the service requests. Therefore, only one URL should needed (unlike IAM, which has a separate service entirely).
Is that correct @mediumTaj? Let me know if I'm missing something or if you have a reason for doing this in Python!
There was a problem hiding this comment.
The url for token exchange looks different from the endpoint for API calls. This was based on the test instance provided:
assistant = AssistantV1(
version='2018-07-10',
username='<username>',
password='<pwd>',
url='https://mycluster.icp:31843/assistant/cfg-default/instances/1559584393/api',
icp4d_url='https://mycluster.icp:31843',
authentication_type='icp4d')
assistant.disable_SSL_verification()
@dpopp07 @mediumTaj please let me know if I understood it wrong
There was a problem hiding this comment.
Oh, interesting. I hadn't seen that yet. Still, I wonder if there is a concrete way to parse the base URL from the service URL to use in the token manager. It seems redundant to pass in both of those when the base is the same. What do you think?
There was a problem hiding this comment.
My 2 cents... while it might be the case that the service endpoint and icp4d token service share a common part of their respective URLs, what happens when the architecture or configuration changes and that is no longer the case? It's probably best to let the user specify the service endpoint URL separate from the ICP4D token service url.
There was a problem hiding this comment.
I too agree with @padamstx to have a separate url for the two.
There was a problem hiding this comment.
Okay, after talking with Phil a bit, I am convinced on accepting two separate URLs. That said, how we specifically handle that might be worth some additional discussion. Let's follow up on this tomorrow
There was a problem hiding this comment.
Let's discuss this on the 6/5 standup call and make sure all SDK cores (and generator) are on the same page with this.
| self.user_access_token = access_token | ||
| self.time_to_live = None | ||
| self.expire_time = None | ||
| self.verify = None # to enable/ disable SSL verification |
There was a problem hiding this comment.
Disabling SSL should actually only be an option in the ICP4D token manager, not both. This is something I changed in the Node PR after initial comments - it was decided not to allow this in the IAM token manager at this point
| if 200 <= response.status_code <= 299: | ||
| return response.json() | ||
| else: | ||
| raise ApiException(response.status_code, http_response=response) |
There was a problem hiding this comment.
Oh I like how you did this with a shared request-launcher function!
| URL = 'url' | ||
| USERNAME = 'username' | ||
| PASSWORD = 'password' | ||
| IAM_APIKEY = 'iam_apikey' | ||
| IAM_URL = 'iam_url' | ||
| ICP4D_URL = 'icp4d_url' |
There was a problem hiding this comment.
Let's discuss this on the 6/5 standup call and make sure all SDK cores (and generator) are on the same page with this.
|
🎉 This PR is included in version 0.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Supports icp4d:
icp4d_access_token,icp4d_urlandauthentication_typeas new params in the constructor of base classdisable_SSL_verificationis propagated to the the JWTTokenManagerICP4DTokenManagerandIAMTokenManagernow inherit fromJWTTokenManager