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

[RHELC-1131, RHELC-1234] Refactor logger to not require root #1029

Merged
merged 3 commits into from
Jan 26, 2024
Merged

[RHELC-1131, RHELC-1234] Refactor logger to not require root #1029

merged 3 commits into from
Jan 26, 2024

Conversation

Venefilyn
Copy link
Member

@Venefilyn Venefilyn commented Jan 10, 2024 •
edited
Loading

To do

  • Potentially changing integration tests
  • Testing locally
  • Fix buffer going to the log file on init

In short of the changes:

  • Split the log handlers into an initialize and finalize function, where FileHandler is added later
  • Logging when not being root no longer causes logs to not work or function correctly (as FileHandler isn't added)
  • Also fixes the issue of clearing the log files if a duplicate process is found, we just initialize the FileHandler later
  • Custom memory buffer is added to keep the last 100 logs, and once FileHandler is added we instantly log the buffer to it

Jira Issues:

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] 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

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 10, 2024
View reviewed changes
convert2rhel/logger.py Fixed Show fixed Hide fixed
convert2rhel/logger.py Fixed Show fixed Hide fixed
@Venefilyn Venefilyn added kind/bug-fix A bug has been fixed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Jan 12, 2024
@has-bot
Copy link
Member

has-bot commented Jan 12, 2024

/packit test --labels tier0

@oamg/conversions-qe please review results and provide ack.

@Venefilyn Venefilyn marked this pull request as ready for review January 12, 2024 17:24
@codecov Codecov
Copy link

codecov bot commented Jan 12, 2024 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c50dd36) 94.33% compared to head (bcfd4ea) 94.26%.
Report is 2 commits behind head on main.

❗ Current head bcfd4ea differs from pull request most recent head a4ebdb0. Consider uploading reports for the commit a4ebdb0 to get more accurate results

Files Patch % Lines
convert2rhel/main.py 80.00% 2 Missing ⚠️
convert2rhel/logger.py 96.96% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   94.33%   94.26%   -0.07%     
==========================================
  Files          47       47              
  Lines        4552     4553       +1     
  Branches      811      815       +4     
==========================================
- Hits         4294     4292       -2     
- Misses        182      183       +1     
- Partials       76       78       +2
Flag Coverage Δ
centos-linux-7 89.44% <93.02%> (-0.07%) ⬇️
centos-linux-8 90.44% <93.02%> (-0.07%) ⬇️
centos-linux-9 90.50% <93.18%> (-0.07%) ⬇️

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 Venefilyn changed the title [RHELC-1131] Refactor logger to not require root [RHELC-1131, RHELC-1234] Refactor logger to not require root Jan 12, 2024
r0x0d
r0x0d reviewed Jan 16, 2024
View reviewed changes
convert2rhel/logger.py Outdated Show resolved Hide resolved
@Venefilyn
Copy link
Member Author

/packit test --labels tier0

@Venefilyn
Copy link
Member Author

/packit test --labels tier0

1 similar comment
@Venefilyn
Copy link
Member Author

/packit test --labels tier0

@Venefilyn
Copy link
Member Author

/packit test --labels tier0

r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/logger.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/logger.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/logger.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/logger.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/main.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/main.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
convert2rhel/utils.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@r0x0d r0x0d 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. Only a few comments & suggestions.

@Venefilyn Venefilyn requested a review from r0x0d January 22, 2024 18:17
@Venefilyn
Copy link
Member Author

@r0x0d addressed all review comments!

@Venefilyn
Copy link
Member Author

/packit test --labels tier0

1 similar comment
@Venefilyn
Copy link
Member Author

/packit test --labels tier0

r0x0d
r0x0d approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@r0x0d r0x0d 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 to me! Thanks for applying the suggestions.

r0x0d
r0x0d approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@r0x0d r0x0d 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 to me! Thanks for applying the suggestions.

Have not tested it under a VM yet.

jochapma
jochapma reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@jochapma jochapma left a comment

Choose a reason for hiding this comment

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

Do we test what happens when we log messages beyond the buffer's capacity?

@Venefilyn
Copy link
Member Author

Venefilyn commented Jan 23, 2024 •
edited
Loading

Do we test what happens when we log messages beyond the buffer's capacity?

@jochapma Yes! I do that here

https://github.com/oamg/convert2rhel/pull/1029/files#diff-df21d2b6de428759d060648d8213d5d3448170b36b47cb5f100149f87cd59a5fR190-R205

@Venefilyn
Copy link
Member Author

Looks good to me! Thanks for applying the suggestions.

Have not tested it under a VM yet.

@r0x0d I have tested it in a VM and the buffer works. It outputs the command used into the logfile

@Venefilyn
Copy link
Member Author

/packit test --labels tier0

@Venefilyn Venefilyn dismissed danmyway’s stale review January 26, 2024 01:01

Code suggestion applied; Tests passing

@Venefilyn Venefilyn merged commit bda20d8 into oamg:main Jan 26, 2024
15 of 58 checks passed
@Venefilyn Venefilyn deleted the refactor/logging branch January 26, 2024 01:01
@Venefilyn Venefilyn mentioned this pull request Feb 22, 2024
Merged
Venefilyn added a commit to Venefilyn/convert2rhel that referenced this pull request Mar 6, 2024
@Venefilyn
fix: errors in utils.py after full test squite
4eb57a8 
Since of merging oamg#1029 we get an issue with unit tests that causes a lot
of strange errors to occur. Likely due to a stream not being closed
correctly.

This aims to either fix or mitigate this from occuring during test runs
as it should not appear in convert2rhel by itself.
@Venefilyn Venefilyn mentioned this pull request Mar 6, 2024
Merged
8 tasks
Venefilyn added a commit to Venefilyn/convert2rhel that referenced this pull request Mar 6, 2024
@Venefilyn
fix: errors in utils.py after full test suite
6e94925 
Since of merging oamg#1029 we get an issue with unit tests that causes a lot
of strange errors to occur. Likely due to a stream not being closed
correctly.

This aims to either fix or mitigate this from occuring during test runs
as it should not appear in convert2rhel by itself.
Venefilyn added a commit that referenced this pull request Mar 7, 2024
@Venefilyn
fix: errors in utils.py after full test suite
474c947 
Since of merging #1029 we get an issue with unit tests that causes a lot
of strange errors to occur. Likely due to a stream not being closed
correctly.

This aims to either fix or mitigate this from occuring during test runs
as it should not appear in convert2rhel by itself.
jochapma pushed a commit to jochapma/convert2rhel that referenced this pull request Mar 11, 2024
@Venefilyn @jochapma
fix: errors in utils.py after full test suite
0167793 
Since of merging oamg#1029 we get an issue with unit tests that causes a lot
of strange errors to occur. Likely due to a stream not being closed
correctly.

This aims to either fix or mitigate this from occuring during test runs
as it should not appear in convert2rhel by itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jochapma jochapma jochapma left review comments

@r0x0d r0x0d r0x0d approved these changes

@danmyway danmyway danmyway left review comments

@pr-watson pr-watson Awaiting requested review from pr-watson

Assignees
No one assigned
Labels
kind/bug-fix A bug has been fixed merge-after-tests-ok tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Venefilyn @has-bot @r0x0d @danmyway @jochapma