Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding logger modules #268

Merged
merged 23 commits into from
May 31, 2024
Merged

Adding logger modules #268

merged 23 commits into from
May 31, 2024

Conversation

pem70
Copy link
Collaborator

@pem70 pem70 commented May 23, 2024 •
edited
Loading

What It Does

Add logger class to core SDK. Fix issue #185.

How to Test

Review Checklist
I certify that I have:

Additional Comments

@codecov Codecov
Copy link

codecov bot commented May 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.67925% with 24 lines in your changes missing coverage. Please review.

Project coverage is 86.21%. Comparing base (d582a88) to head (5ecbeab).
Report is 254 commits behind head on main.

Files with missing lines Patch % Lines
src/core/zowe/core_for_zowe_sdk/profile_manager.py 42.10% 11 Missing ⚠️
src/zos_files/zowe/zos_files_for_zowe_sdk/files.py 72.00% 7 Missing ⚠️
src/core/zowe/core_for_zowe_sdk/config_file.py 88.23% 2 Missing ⚠️
src/core/zowe/core_for_zowe_sdk/zosmf_profile.py 50.00% 2 Missing ⚠️
src/zos_jobs/zowe/zos_jobs_for_zowe_sdk/jobs.py 71.42% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
+ Coverage   84.88%   86.21%   +1.32%     
==========================================
  Files          34       35       +1     
  Lines        2124     2292     +168     
==========================================
+ Hits         1803     1976     +173     
+ Misses        321      316       -5
Flag Coverage Δ
unittests 86.21% <88.67%> (+1.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pem70 added 11 commits May 23, 2024 13:35
@pem70
modified logger logic and stream request
4d1fa81 
Signed-off-by: pem70 <[email protected]>
@pem70
Add log class and set logger level
4351310 
Signed-off-by: pem70 <[email protected]>
@pem70
Basic unittesting
c1f9f1e 
Signed-off-by: pem70 <[email protected]>
@pem70
update unittest
f7d261e 
Signed-off-by: pem70 <[email protected]>
@pem70
Add more unittests on logger
c7060a7 
Signed-off-by: pem70 <[email protected]>
@pem70
unit test on config_file.py
c99b0b0 
Signed-off-by: pem70 <[email protected]>
@pem70
update unit test
d6c6c31 
Signed-off-by: pem70 <[email protected]>
@pem70
update unit test
47e34f4 
Signed-off-by: pem70 <[email protected]>
@pem70
Add description for test functions
046a4c7 
Signed-off-by: pem70 <[email protected]>
@pem70
update changelog
0f36770 
Signed-off-by: pem70 <[email protected]>
@pem70
Update CHANGELOG.md
1597f36 
Signed-off-by: pem70 <[email protected]>
@pem70 pem70 marked this pull request as ready for review May 28, 2024 17:25
@pem70 pem70 linked an issue May 28, 2024 that may be closed by this pull request
Add Logger class to core SDK #185
Closed
4 tasks
@pem70 pem70 requested review from t1m0thyj, zFernand0 and traeok May 28, 2024 17:29
t1m0thyj
t1m0thyj requested changes May 29, 2024
View reviewed changes
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, thanks @pem70 for your contribution! Left a few comments.

.vscode/settings.json Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/config_file.py Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/request_handler.py Show resolved Hide resolved
pem70 and others added 3 commits May 29, 2024 08:56
@pem70 @t1m0thyj
Update CHANGELOG.md
7be42b1 
Co-authored-by: Timothy Johnson <[email protected]>
Signed-off-by: pem70 <[email protected]>
@pem70 @t1m0thyj
Update src/core/zowe/core_for_zowe_sdk/logger.py
231296f 
Co-authored-by: Timothy Johnson <[email protected]>
Signed-off-by: pem70 <[email protected]>
@pem70 @t1m0thyj
Update src/core/zowe/core_for_zowe_sdk/logger.py
cf4e914 
Co-authored-by: Timothy Johnson <[email protected]>
Signed-off-by: pem70 <[email protected]>
pem70
pem70 commented May 29, 2024
View reviewed changes
Copy link
Collaborator Author

@pem70 pem70 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! I don't find an urgent need for fixing these changes asap and they could be addressed in separate issues. However, if you think them should be done right now, I can still do it.

src/core/zowe/core_for_zowe_sdk/config_file.py Show resolved Hide resolved
.vscode/settings.json Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/logger.py Show resolved Hide resolved
pem70
pem70 commented May 29, 2024
View reviewed changes
Copy link
Collaborator Author

@pem70 pem70 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solve the bug in RegisterLogger(name: str)

@JTonda JTonda requested a review from t1m0thyj May 29, 2024 15:18
@JTonda JTonda requested a review from awharn May 29, 2024 15:19
traeok
traeok reviewed May 29, 2024
View reviewed changes
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, thanks @pem70! I left a couple comments regarding error messaging

src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/credential_manager.py
@@ -49,6 +53,7 @@ def load_secure_props() -> None:
return

except Exception as exc:
CredentialManager.__logger.error(f"Fail to load secure profile {constants['ZoweServiceName']}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CredentialManager.__logger.error(f"Fail to load secure profile {constants['ZoweServiceName']}")
CredentialManager.__logger.error(f"Failed to load secure profile {constants['ZoweServiceName']}")

src/core/zowe/core_for_zowe_sdk/logger.py Outdated Show resolved Hide resolved
src/core/zowe/core_for_zowe_sdk/profile_manager.py Outdated Show resolved Hide resolved
This was referenced May 29, 2024
Open
Open
Open
t1m0thyj
t1m0thyj reviewed May 29, 2024
View reviewed changes
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left one more comment that is optional, and will approve once Trae's comments have been addressed 😋

src/core/zowe/core_for_zowe_sdk/profile_manager.py Outdated
Comment on lines 62 to 63
self.__logger = logging.getLogger(__name__)
Log.registerLogger(__name__)
Copy link
Member

@t1m0thyj t1m0thyj May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the registerLogger function 👍 Sorry I wasn't very clear in my previous comment, but would it make sense to have the registerLogger function return the same logger object that is returned by getLogger? That would allow us to condense these 2 lines into just 1:

Suggested change
self.__logger = logging.getLogger(__name__)
Log.registerLogger(__name__)
self.__logger = Log.registerLogger(__name__)

pem70 and others added 3 commits May 30, 2024 10:10
pem70
pem70 commented May 30, 2024
View reviewed changes
Copy link
Collaborator Author

@pem70 pem70 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the comments!

t1m0thyj
t1m0thyj approved these changes May 30, 2024
View reviewed changes
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @pem70!

traeok
traeok approved these changes May 30, 2024
View reviewed changes
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the logger implementation @pem70! 😁

@traeok traeok merged commit 5816aab into main May 31, 2024
21 checks passed
@traeok traeok deleted the logger branch May 31, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traeok traeok traeok approved these changes

@t1m0thyj t1m0thyj t1m0thyj approved these changes

@zFernand0 zFernand0 Awaiting requested review from zFernand0

@awharn awharn Awaiting requested review from awharn

Assignees

@pem70 pem70

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Logger class to core SDK
3 participants
@pem70 @t1m0thyj @traeok