[RHELC-1732] refactor: Improved logger usage #1327

Venefilyn · 2024-08-02T13:00:42Z

This refactor of the usage of our logging is to get away from custom
functions that causes attribute errors in editors and IDEs. This will
also standarize the usage of logger and the loglevels.

In future PRs we can include changes to include prepare, rollback, and
final specific message styles.

Jira Issues:

RHELC-1732

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] or [HMS-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

convert2rhel/logger.py

convert2rhel/utils/__init__.py

convert2rhel/backup/certs.py

convert2rhel/toolopts.py

convert2rhel/logger.py

codecov · 2024-08-19T15:20:49Z

Codecov Report

Attention: Patch coverage is 96.11872% with 17 lines in your changes missing coverage. Please review.

Project coverage is 96.34%. Comparing base (6722626) to head (11b8e34).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
convert2rhel/subscription.py	94.59%	4 Missing ⚠️
convert2rhel/logger.py	93.18%	1 Missing and 2 partials ⚠️
convert2rhel/utils/__init__.py	90.90%	3 Missing ⚠️
convert2rhel/pkghandler.py	90.47%	2 Missing ⚠️
convert2rhel/backup/packages.py	95.00%	1 Missing ⚠️
convert2rhel/breadcrumbs.py	83.33%	1 Missing ⚠️
convert2rhel/pkgmanager/handlers/yum/__init__.py	96.00%	1 Missing ⚠️
convert2rhel/redhatrelease.py	83.33%	1 Missing ⚠️
convert2rhel/toolopts.py	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
- Coverage   96.43%   96.34%   -0.10%     
==========================================
  Files          66       66              
  Lines        4997     4983      -14     
  Branches      877      877              
==========================================
- Hits         4819     4801      -18     
- Misses        101      102       +1     
- Partials       77       80       +3

Flag	Coverage Δ
centos-linux-7	`91.76% <89.72%> (-0.11%)`	⬇️
centos-linux-8	`92.64% <90.86%> (-0.11%)`	⬇️
centos-linux-9	`92.69% <90.86%> (-0.11%)`	⬇️

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.

Venefilyn · 2024-08-19T15:45:55Z

Ready for review and testing whenever (wanted it to be ready before I leave for PTO)

Venefilyn · 2024-08-20T15:10:33Z

convert2rhel/logger.py

+# get root logger
+logging.setLoggerClass(CustomLogger)
+root_logger = logging.getLogger("convert2rhel")  # type: CustomLogger # type: ignore


This can look a bit confusing but essentially we set it to a type explicitly, but since getLogger() just returns a normal Logger class even if we set our own we have to ignore it as well. It is something that could be improved with more code if it doesn't work for everyone, but with this we get what we want and it should be safe given how the setLoggerClass() works

This implementation is a bit different from how the demo (recording exists) that I had August 13th

Looks fine for me. Not a single problem

convert2rhel/logger.py

convert2rhel/actions/conversion/transaction.py

convert2rhel/actions/conversion/preserve_only_rhel_kernel.py

convert2rhel/actions/pre_ponr_changes/backup_system.py

convert2rhel/logger.py

has-bot · 2024-09-10T15:28:52Z

/packit test --labels sanity

Comment generated by an automation.

Log | Bot Usage

SerCantus

Awesome refactor @Venefilyn 🥇 . I added the tests-run-sanity label to the PR.

convert2rhel/logger.py

SerCantus · 2024-09-11T11:42:27Z

Looks like sanity checks are failing as the test doesn't recognise the convert2rhel module importing root_logger in tests/integration/conftest.py. Do you know if the integration tests should use the same logger as C2R?

Venefilyn · 2024-09-11T12:06:42Z

Looks like sanity checks are failing as the test doesn't recognise the convert2rhel module importing root_logger in tests/integration/conftest.py. Do you know if the integration tests should use the same logger as C2R?

@SerCantus good spot. I don't think I should change the integration tests. I changed it back now and will re-trigger tests

r0x0d · 2024-09-11T12:44:12Z

convert2rhel/logger.py

+        new_fmt = fmt_orig if not color or self.color_disabled else colorize(fmt_orig, color)
+        self._fmt = new_fmt


What do you think unpacking the if-else one-liner to this?

In my opinion, it makes easier to read when we have a if with more than one statement like the current one.

Suggested change

new_fmt = fmt_orig if not color or self.color_disabled else colorize(fmt_orig, color)

self._fmt = new_fmt

new_fmt = fmt_orig

if color or not self.color_disaled:

new_fmt = colorize(fmt_orig, color)

self._fmt = new_fmt

Not a required change, to be honest, just posting in case if you agree to apply.

r0x0d · 2024-09-11T12:46:40Z

convert2rhel/logger.py

+class CustomLogger(logging.Logger):
+    def __init__(self, name, level=0):


Can we have a docstring here mentioning why this class exist? Looking at it and understanding the context of the PR is easy to guess, but future people might not get it at first.

Especially if you can include that the critical_no_exit, task and file were moved here to avoid messing up with the parent logger class

r0x0d · 2024-09-11T13:03:09Z

/packit test --labels sanity

This refactor of the usage of our logging is to get away from custom functions that causes attribute errors in editors and IDEs. This will also standarize the usage of logger and the loglevels. In future PRs we can include changes to include prepare, rollback, and final specific message styles.

Venefilyn · 2024-09-11T15:25:36Z

@r0x0d Addressed your comments, WDYT?

Venefilyn · 2024-09-11T15:52:23Z

/packit test --labels sanity

Venefilyn marked this pull request as draft August 2, 2024 13:00

Venefilyn force-pushed the refactor/logger branch from 1119b7d to bd5a8f5 Compare August 2, 2024 15:30

github-advanced-security bot found potential problems Aug 2, 2024

View reviewed changes

Venefilyn force-pushed the refactor/logger branch 2 times, most recently from 246d94f to d97b77e Compare August 19, 2024 15:17

github-advanced-security bot found potential problems Aug 19, 2024

View reviewed changes

convert2rhel/logger.py Fixed Show fixed Hide fixed

Venefilyn force-pushed the refactor/logger branch from d97b77e to f0a0696 Compare August 19, 2024 15:22

Venefilyn marked this pull request as ready for review August 19, 2024 15:22

Venefilyn requested review from a team as code owners August 19, 2024 15:22

Venefilyn requested a review from SerCantus August 19, 2024 15:22

Venefilyn force-pushed the refactor/logger branch from f0a0696 to 2995d32 Compare August 19, 2024 15:23

Venefilyn changed the title ~~Standardize logger usage~~ refactor: Improved logger usage Aug 19, 2024

Venefilyn added the skip/changelog If it should be excluded from changelog or Release notes. Such as infra, reverted PRs, etc. label Aug 19, 2024

Venefilyn force-pushed the refactor/logger branch from 2995d32 to 33f5ded Compare August 19, 2024 15:27

Venefilyn assigned r0x0d and SerCantus Aug 20, 2024

Venefilyn commented Aug 20, 2024

View reviewed changes

convert2rhel/logger.py Show resolved Hide resolved

Venefilyn commented Aug 20, 2024

View reviewed changes

convert2rhel/logger.py Show resolved Hide resolved

Venefilyn force-pushed the refactor/logger branch 3 times, most recently from 8e56069 to c01f078 Compare August 21, 2024 17:51

Venefilyn force-pushed the refactor/logger branch from c6ea6c2 to 2b64ab9 Compare September 2, 2024 14:34

SerCantus reviewed Sep 3, 2024

View reviewed changes

Venefilyn force-pushed the refactor/logger branch 3 times, most recently from 75a55dd to 9ad27b6 Compare September 10, 2024 11:30

Venefilyn force-pushed the refactor/logger branch 3 times, most recently from b51f620 to a9c80df Compare September 10, 2024 11:50

Venefilyn changed the title ~~refactor: Improved logger usage~~ [RHELC-1732] refactor: Improved logger usage Sep 10, 2024

Venefilyn force-pushed the refactor/logger branch from a9c80df to 2c61116 Compare September 10, 2024 14:47

Venefilyn mentioned this pull request Sep 10, 2024

refactor: Improved logger usage #1373

Merged

8 tasks

SerCantus added the tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. label Sep 10, 2024

SerCantus approved these changes Sep 10, 2024

View reviewed changes

convert2rhel/logger.py Show resolved Hide resolved

r0x0d mentioned this pull request Sep 10, 2024

[RHELC-1507] Port environment variables to config file #1272

Merged

8 tasks

Venefilyn force-pushed the refactor/logger branch from 2c61116 to ac3a5d5 Compare September 11, 2024 12:05

Venefilyn force-pushed the refactor/logger branch from ac3a5d5 to ab5be1a Compare September 11, 2024 12:31

r0x0d reviewed Sep 11, 2024

View reviewed changes

Venefilyn force-pushed the refactor/logger branch from ab5be1a to 01305ea Compare September 11, 2024 15:22

Venefilyn added 2 commits September 11, 2024 15:24

refactor: move codebase to new logging syntax

11b8e34

Venefilyn force-pushed the refactor/logger branch from 01305ea to 11b8e34 Compare September 11, 2024 15:24

r0x0d approved these changes Sep 11, 2024

View reviewed changes

r0x0d merged commit 6768a26 into oamg:main Sep 11, 2024
27 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1732] refactor: Improved logger usage #1327

[RHELC-1732] refactor: Improved logger usage #1327

Venefilyn commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

Venefilyn commented Aug 19, 2024

Venefilyn Aug 20, 2024 •

edited

Loading

r0x0d Sep 11, 2024

has-bot commented Sep 10, 2024

SerCantus left a comment

SerCantus commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

r0x0d Sep 11, 2024

r0x0d Sep 11, 2024

r0x0d Sep 11, 2024

r0x0d commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

		new_fmt = fmt_orig if not color or self.color_disabled else colorize(fmt_orig, color)
		self._fmt = new_fmt

		class CustomLogger(logging.Logger):
		def __init__(self, name, level=0):

[RHELC-1732] refactor: Improved logger usage #1327

[RHELC-1732] refactor: Improved logger usage #1327

Conversation

Venefilyn commented Aug 2, 2024 • edited Loading

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

Venefilyn commented Aug 19, 2024

Venefilyn Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

r0x0d Sep 11, 2024

Choose a reason for hiding this comment

has-bot commented Sep 10, 2024

SerCantus left a comment

Choose a reason for hiding this comment

SerCantus commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

r0x0d Sep 11, 2024

Choose a reason for hiding this comment

r0x0d Sep 11, 2024

Choose a reason for hiding this comment

r0x0d Sep 11, 2024

Choose a reason for hiding this comment

r0x0d commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

Venefilyn commented Sep 11, 2024

Venefilyn commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

Venefilyn Aug 20, 2024 •

edited

Loading