refactor: use shared code for IAM based authenticators by pyrooka · Pull Request #118 · IBM/python-sdk-core

pyrooka · 2021-08-05T16:22:55Z

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 · 2021-08-05T16:23:05Z

Codecov Report

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

@@            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%)`	⬆️
... 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 · 2021-08-05T17:54:06Z

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.

padamstx · 2021-08-05T18:08:23Z

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

padamstx

Looks good! I spotted one minor thing that needs to be changed but I'll approve now to avoid a re-review. It looks like this refactoring work will greatly reduce the amount of new code needed for the ContainerAuthenticator and (later on) the VPCInstanceAuthenticator. I've found the same thing in the Java core as well and I suspect @dpopp07 is seeing the same in the Node core. @dpopp07 Thank you for the idea to do this refactoring :)

padamstx · 2021-08-05T18:09:42Z

+    Attributes:
+        token_manager (TokenManager): Retrives and manages IAM tokens from the endpoint specified by the url.
+    """
+    authentication_type = 'iam'


This won't apply to all IAM-based authenticators, so it probably belongs only in the IamAuthenticator class.

dpopp07

I second Phil's comment but otherwise, looks good! 👍

More info: - Surround top-level function and class definitions with two blank lines: https://www.python.org/dev/peps/pep-0008/#blank-lines - Imports grouping and ordering: https://www.python.org/dev/peps/pep-0008/#imports

pyrooka · 2021-08-06T11:30:28Z

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:

Surround top-level function and class definitions with two blank lines: https://www.python.org/dev/peps/pep-0008/#blank-lines
Imports grouping and ordering: https://www.python.org/dev/peps/pep-0008/#imports

(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.)

padamstx

Latest changes LGTM!

padamstx · 2021-08-06T14:02:46Z

        """
        headers = {
-            'Content-type': 'applicaton/x-www-form-urlencoded',
+            'Content-type': 'application/x-www-form-urlencoded',


wow, the misspelled "applicaton" didn't cause a problem before??? :)

I was surprised too! The container authenticator tests will cover this, so I didn't change the tests here.

ibm-devx-sdk · 2021-08-12T15:38:41Z

🎉 This PR is included in version 3.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

pyrooka added 2 commits August 5, 2021 17:54

refactor: shared token manager class with the IAM related functions

7fcf4f2

refactor: shared authenticator class with the IAM related functions

82c770b

test: cover missing line (set_disable_ssl_verification)

84cc59e

pyrooka requested review from padamstx and rmkeezer August 5, 2021 17:54

padamstx approved these changes Aug 5, 2021

View reviewed changes

padamstx requested a review from dpopp07 August 5, 2021 18:21

dpopp07 approved these changes Aug 5, 2021

View reviewed changes

pyrooka added 3 commits August 6, 2021 12:13

chore: remove auth type from the shared IAM class

3e73ce2

chore: restructure token managers into a directory

0568556

refactor: rearrange imports and add missing blank lines

a4d9525

More info: - Surround top-level function and class definitions with two blank lines: https://www.python.org/dev/peps/pep-0008/#blank-lines - Imports grouping and ordering: https://www.python.org/dev/peps/pep-0008/#imports

fix: typo

9b73630

pyrooka mentioned this pull request Aug 6, 2021

feat: implement container authentication #119

Merged

padamstx approved these changes Aug 6, 2021

View reviewed changes

pyrooka merged commit e0aeed7 into main Aug 6, 2021

pyrooka deleted the shared-iam-related-code branch August 6, 2021 15:52

ibm-devx-sdk added the released label Aug 12, 2021

vmatyus mentioned this pull request Aug 13, 2021

build(deps): Bump ibm-cloud-sdk-core from 3.10.1 to 3.11.0 IBM/cloudant-python-sdk#130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use shared code for IAM based authenticators#118

refactor: use shared code for IAM based authenticators#118
pyrooka merged 7 commits intomainfrom
shared-iam-related-code

pyrooka commented Aug 5, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Aug 5, 2021 •

edited

Loading

Uh oh!

pyrooka commented Aug 5, 2021

Uh oh!

padamstx commented Aug 5, 2021 •

edited

Loading

Uh oh!

padamstx left a comment •

edited

Loading

Uh oh!

padamstx Aug 5, 2021

Uh oh!

dpopp07 left a comment

Uh oh!

pyrooka commented Aug 6, 2021 •

edited

Loading

Uh oh!

padamstx left a comment

Uh oh!

padamstx Aug 6, 2021

Uh oh!

pyrooka Aug 6, 2021

Uh oh!

ibm-devx-sdk commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

pyrooka commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pyrooka commented Aug 5, 2021

Uh oh!

padamstx commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

padamstx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

padamstx Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

pyrooka commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

padamstx Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

pyrooka Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

ibm-devx-sdk commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pyrooka commented Aug 5, 2021 •

edited

Loading

codecov Bot commented Aug 5, 2021 •

edited

Loading

padamstx commented Aug 5, 2021 •

edited

Loading

padamstx left a comment •

edited

Loading

pyrooka commented Aug 6, 2021 •

edited

Loading