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-1329] Port pkghandler.preserve_only_rhel_kernel() to Action framework #1250

Merged
merged 17 commits into from
Aug 7, 2024
Merged

[RHELC-1329] Port pkghandler.preserve_only_rhel_kernel() to Action framework #1250

merged 17 commits into from
Aug 7, 2024

Conversation

pr-watson
Copy link
Contributor

@pr-watson pr-watson commented May 31, 2024 •
edited
Loading

This PR ports the function preserve_only_rhel_kernel to the action framework. Changing any relevant logger crictical, warning and info messages to Error results and Warning and Info messages respectively.

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

@pr-watson pr-watson requested a review from r0x0d May 31, 2024 20:57
@pr-watson pr-watson marked this pull request as draft May 31, 2024 20:58
@pr-watson
Copy link
Contributor Author

@r0x0d will add the tests after we confirm that this structure is okay for the port

@pr-watson pr-watson added kind/feature New feature or request tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. labels May 31, 2024
@has-bot
Copy link
Member

has-bot commented May 31, 2024

/packit test --labels sanity

Comment generated by an automation.

r0x0d
r0x0d reviewed Jun 4, 2024
View reviewed changes
convert2rhel/actions/conversion/preserve_only_rhel_kernel.py
)


class FixInvalidGrub2Entries(actions.Action):
Copy link
Member

Choose a reason for hiding this comment

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

This looks to me that it should probably belongs to a grub.py module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True I could see it as apart of the grub refactor we were going to do - seems important to preserving the rhel kernel though

@codecov Codecov
Copy link

codecov bot commented Jun 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.41379% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.13%. Comparing base (703e2b3) to head (d39ebc4).

Files Patch % Lines
...el/actions/conversion/preserve_only_rhel_kernel.py 97.41% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1250      +/-   ##
==========================================
+ Coverage   95.92%   96.13%   +0.21%     
==========================================
  Files          56       57       +1     
  Lines        4784     4818      +34     
  Branches      750      846      +96     
==========================================
+ Hits         4589     4632      +43     
+ Misses        111      107       -4     
+ Partials       84       79       -5
Flag Coverage Δ
centos-linux-7 91.39% <97.41%> (+0.25%) ⬆️
centos-linux-8 92.31% <97.41%> (?)
centos-linux-9 92.35% <97.41%> (+0.24%) ⬆️

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.

r0x0d
r0x0d reviewed Jun 26, 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.

Some small comments, otherwise looks good!

@pr-watson pr-watson force-pushed the migrate-post-conversion-preserve-only-rhel-kernel branch from 58929f5 to e30a880 Compare July 3, 2024 20:16
@pr-watson pr-watson marked this pull request as ready for review July 8, 2024 12:59
@pr-watson
Copy link
Contributor Author

/packit build

@pr-watson pr-watson requested a review from a team as a code owner August 6, 2024 16:04
@r0x0d
Copy link
Member

r0x0d commented Aug 6, 2024

/packit build

r0x0d
r0x0d reviewed Aug 6, 2024
View reviewed changes
convert2rhel/actions/conversion/preserve_only_rhel_kernel.py
Comment on lines +56 to +83
already_installed = re.search(r" (.*?)(?: is)? already installed", output, re.MULTILINE)
if already_installed:
rhel_kernel_nevra = already_installed.group(1)
non_rhel_kernels = pkghandler.get_installed_pkgs_w_different_fingerprint(
system_info.fingerprints_rhel, "kernel"
)
for non_rhel_kernel in non_rhel_kernels:
# We're comparing to NEVRA since that's what yum/dnf prints out
if rhel_kernel_nevra == pkghandler.get_pkg_nevra(non_rhel_kernel):
# If the installed kernel is from a third party (non-RHEL) and has the same NEVRA as the one available
# from RHEL repos, it's necessary to install an older version RHEL kernel and the third party one will
# be removed later in the conversion process. It's because yum/dnf is unable to reinstall a kernel.
info_message = (
"Conflict of kernels: One of the installed kernels"
" has the same version as the latest RHEL kernel."
)
loggerinst.info("\n%s" % info_message)
self.add_message(
level="INFO",
id="CONFLICT_OF_KERNELS",
title="Conflict of installed kernel versions",
description=info_message,
)
pkghandler.handle_no_newer_rhel_kernel_available()
kernel_update_needed = True

if kernel_update_needed:
pkghandler.update_rhel_kernel()
Copy link
Member

Choose a reason for hiding this comment

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

We have a discussion tomorrow about this code, and if we decide that @danmyway solution is the way to go, then I think we can incorporate his changes here: https://github.com/oamg/convert2rhel/pull/1323/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

r0x0d
r0x0d approved these changes Aug 6, 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

@r0x0d
Copy link
Member

r0x0d commented Aug 6, 2024

/packit test --labels sanity

@r0x0d r0x0d dismissed Venefilyn’s stale review August 7, 2024 14:49

tests passed and one waived. code looks good and we can iterate on next PRs if needed.

pr-watson and others added 17 commits August 7, 2024 10:49
@pr-watson
[RHELC-1329] Port pkghandler.preserve_only_rhel_kernel() to Action fr…
1c4d666 
…amework
@pr-watson @r0x0d
Apply suggestions from code review
066db73 
Co-authored-by: Rodolfo Olivieri <[email protected]>
@pr-watson
Code review suggestions
8a01104
@pr-watson
Unit tests
327eb5f
@pr-watson
Updated unit tests
f3bf636
@pr-watson @r0x0d
Apply suggestions from code review
4e78178 
Co-authored-by: Rodolfo Olivieri <[email protected]>
@pr-watson
Update title
bd3126b
@pr-watson
Fix unit tests
bd8a4ad
@pr-watson @r0x0d
Apply suggestions from code review
e2c5f89 
Co-authored-by: Rodolfo Olivieri <[email protected]>
@pre-commit-ci @pr-watson
[pre-commit.ci] auto fixes from pre-commit.com hooks
6e93da6 
for more information, see https://pre-commit.ci
@pr-watson
Fix unit tests
39971a2
@pr-watson
Remove preserve_only_rhel_kernel call from main
b6722ce
@pr-watson @r0x0d
Apply suggestions from code review
948fb24 
Co-authored-by: Rodolfo Olivieri <[email protected]>
@pr-watson
Update RHEL_KERNEL_INSTALLED info block
58ca086
@pr-watson
Update info log for kernel install
643f839
@pr-watson
Fix unit tests
d29c3a3
@pr-watson
Add unit test for invalid grub2 entries
d39ebc4
@pr-watson pr-watson force-pushed the migrate-post-conversion-preserve-only-rhel-kernel branch from 59566b8 to d39ebc4 Compare August 7, 2024 14:49
@r0x0d r0x0d enabled auto-merge (squash) August 7, 2024 14:49
@r0x0d r0x0d merged commit 3d3dbf7 into oamg:main Aug 7, 2024
15 of 18 checks passed
pr-watson added a commit to pr-watson/convert2rhel that referenced this pull request Aug 13, 2024
@pr-watson @r0x0d @pre-commit-ci
[RHELC-1329] Port pkghandler.preserve_only_rhel_kernel() to Action fr…
e868643 
…amework (oamg#1250)

* [RHELC-1329] Port pkghandler.preserve_only_rhel_kernel() to Action framework

* Apply suggestions from code review

Co-authored-by: Rodolfo Olivieri <[email protected]>

* Code review suggestions

* Unit tests

* Updated unit tests

* Apply suggestions from code review

Co-authored-by: Rodolfo Olivieri <[email protected]>

* Update title

* Fix unit tests

* Apply suggestions from code review

Co-authored-by: Rodolfo Olivieri <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix unit tests

* Remove preserve_only_rhel_kernel call from main

* Apply suggestions from code review

Co-authored-by: Rodolfo Olivieri <[email protected]>

* Update RHEL_KERNEL_INSTALLED info block

* Update info log for kernel install

* Fix unit tests

* Add unit test for invalid grub2 entries

---------

Co-authored-by: Rodolfo Olivieri <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This was referenced Aug 21, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r0x0d r0x0d r0x0d approved these changes

@Venefilyn Venefilyn Venefilyn left review comments

Assignees
No one assigned
Labels
kind/feature New feature or request merge-after-tests-ok tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pr-watson @has-bot @r0x0d @Venefilyn