IBM / python-sdk-core

The python-sdk-core repository contains core functionality required by Python code generated by the IBM OpenAPI SDK Generator.
Apache License 2.0
20 stars 27 forks source link

refactor: use shared code for IAM based authenticators #118

Closed pyrooka closed 3 years ago

pyrooka commented 3 years ago

This PR introduces two new classes: IAMRequestBasedTokenManager and IAMRequestBasedAuthenticator. These contain common functionalities for IAM auth based flows.

I haven't changed the tests on purpose. (Well, there is a minor change to fix the codecov patch check). The reason is to make sure that no interface change has been made, so everything is working just like before.

codecov[bot] commented 3 years ago

Codecov Report

Merging #118 (9b73630) into main (814a596) will increase coverage by 0.14%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   98.77%   98.92%   +0.14%     
==========================================
  Files          18       20       +2     
  Lines         737      746       +9     
==========================================
+ Hits          728      738      +10     
+ Misses          9        8       -1     
Impacted Files Coverage Δ
ibm_cloud_sdk_core/authenticators/authenticator.py 75.00% <ø> (ø)
..._core/authenticators/bearer_token_authenticator.py 100.00% <ø> (ø)
...d_sdk_core/authenticators/no_auth_authenticator.py 100.00% <ø> (ø)
ibm_cloud_sdk_core/get_authenticator.py 100.00% <ø> (ø)
...loud_sdk_core/token_managers/cp4d_token_manager.py 100.00% <ø> (ø)
ibm_cloud_sdk_core/utils.py 99.33% <ø> (ø)
ibm_cloud_sdk_core/__init__.py 100.00% <100.00%> (ø)
ibm_cloud_sdk_core/api_exception.py 100.00% <100.00%> (ø)
...loud_sdk_core/authenticators/cp4d_authenticator.py 100.00% <100.00%> (ø)
...cloud_sdk_core/authenticators/iam_authenticator.py 100.00% <100.00%> (+3.12%) :arrow_up:
... and 9 more

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 814a596...9b73630. Read the comment docs.

pyrooka commented 3 years ago

I am thinking about one more thing that could improve the project structure: put all the token managers into a single directory, just like the authenticators. Not sure would this break anything or not, I only found this. Screenshot 2021-08-05 at 19 13 29

padamstx commented 3 years ago

Not sure would this break anything or not,

if you move the token manager files to a new "token_managers" directory, couldn't you just update the ibm_cloud_sdk_core/__init__.py file like the snippet below to import them from the new location and effectively preserve the fact that they can be imported by users from the ibm_cloud_sdk_core package?

from token_managers.iam_token_manager import IAMTokenManager
from token_managers.jwt_token_manager import JWTTokenManager
from token_managers.cp4d_token_manager import CP4DTokenManager
pyrooka commented 3 years ago

I've pushed ~3~ a few more commits since your review. The first 2 contain the authentication_type = 'iam' fix and the restructuring of the token managers. The ~last~ third commit is really just about the code style. I found that there was a bit inconsistency so I checked every single file, reordered/grouped the imports and added missing blank lines (where it was necessary).

More info:

(Since there is a lot of changes (number of files) in the last commit, it makes the overall diff a bit messy, sorry for that! I didn't want to create a separate PR for this.)

ibm-devx-sdk commented 3 years ago

:tada: This PR is included in version 3.11.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: