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-126] Stop if the booted kernel is not owned by any package #424

Merged
merged 1 commit into from
Jul 13, 2022
Merged

[RHELC-126] Stop if the booted kernel is not owned by any package #424

merged 1 commit into from
Jul 13, 2022

Conversation

Andrew-ang9
Copy link
Contributor

@Andrew-ang9 Andrew-ang9 commented Feb 4, 2022 •
edited by Venefilyn
Loading

adds a unit test to catch where the tag "{{NAME}}" has an extra set of curly brace.

This error was fixed in the PR #389

if you'd like to test this use the commit hash 31a304c to use the istant with the error
and use commit hash 9a1c5f5 for the fix for this error

Jira ticket: https://issues.redhat.com/browse/RHELC-126

@abadger abadger changed the title fixes the tag for query format Add a unit test for the rpm query format in bad_kernel_package_signature Feb 4, 2022
Venefilyn
Venefilyn requested changes Feb 9, 2022
View reviewed changes
convert2rhel/unit_tests/checks_test.py Outdated Show resolved Hide resolved
@abadger
Copy link
Member

abadger commented Feb 15, 2022

This now looks good to me (but I have been helping Andrew to write it so another pair of eyes is welcome).

r0x0d
r0x0d reviewed Feb 16, 2022
View reviewed changes
convert2rhel/checks.py Outdated Show resolved Hide resolved
r0x0d
r0x0d reviewed Feb 16, 2022
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.

Looking good, one minor comment about the code.

bocekm
bocekm requested changes Feb 16, 2022
View reviewed changes
convert2rhel/checks.py Outdated Show resolved Hide resolved
abadger
abadger reviewed Feb 23, 2022
View reviewed changes
convert2rhel/checks.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 9, 2022 •
edited
Loading

Codecov Report

Merging #424 (3a71e86) into main (30dcc73) will increase coverage by 0.02%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
+ Coverage   87.51%   87.53%   +0.02%     
==========================================
  Files          17       17              
  Lines        2419     2423       +4     
  Branches      418      419       +1     
==========================================
+ Hits         2117     2121       +4     
  Misses        241      241              
  Partials       61       61
Impacted Files Coverage Δ
convert2rhel/checks.py 95.47% <100.00%> (+0.08%) ⬆️

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 30dcc73...c6c3040. Read the comment docs.

Venefilyn
Venefilyn requested changes Mar 10, 2022
View reviewed changes
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Show resolved Hide resolved
r0x0d
r0x0d reviewed Apr 12, 2022
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.

Pretty good work, @Andrew-ang9! I just left some minor comments for you that might improve the readability of the code in general!

convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/unit_tests/checks_test.py Show resolved Hide resolved
convert2rhel/unit_tests/checks_test.py Show resolved Hide resolved
@kokesak kokesak mentioned this pull request Jun 23, 2022
Closed
Venefilyn
Venefilyn previously approved these changes Jun 29, 2022
View reviewed changes
@Venefilyn Venefilyn requested a review from bocekm July 7, 2022 10:36
@danmyway
Copy link
Member

@SpyTec, @bocekm I suppose we will not need an integration test for this, right?

@bocekm
Copy link
Member

bocekm commented Jul 11, 2022 •
edited
Loading

@danmyway, there is an added functionality which will catch the situation where the booted kernel (vmlinuz) is not owned by any installed package. I'd say it's quite an unlikely situation to happen though. From what I know even if you build a custom kernel you build an rpm package which you then install. So even then the kernel is owned by some package.

What you'd need to do to simulate the situation is backing up all the kernel package files, removing the kernel package, and then restoring the files from a backup. Or tweak the rpm database.

All in all, I don't think it's worth covering this piece of code by an integration test.

@abadger, let us know if you think otherwise.

bocekm
bocekm previously approved these changes Jul 11, 2022
View reviewed changes
@abadger
Copy link
Member

abadger commented Jul 11, 2022

I agree with @bocekm. The additonal case is likely to be encountered extremely infrequently.

abadger
abadger previously approved these changes Jul 11, 2022
View reviewed changes
@danmyway
Copy link
Member

Alright, thank you, @bocekm and @abadger !
I will keep it in backlog just in case.
If you could rebase to upstream @Andrew-ang9, please, I think we're ready for merge then.

@bocekm bocekm changed the title Add a unit test for the rpm query format in bad_kernel_package_signature Stop if the booted kernel is not owned by any package Jul 12, 2022
@Andrew-ang9 Andrew-ang9 dismissed stale reviews from abadger, bocekm, and Venefilyn via 3a71e86 July 12, 2022 18:48
@Andrew-ang9
added a check to the unit test to make sure that run_subprocess is be…
c6c3040 
…ing called with the right arguments

* This check is to fix that if the rpm qurery format is wrong it will catch it
* This fix is not the best because if the formating is wrong in the unit test and the code then the test might pass but still not be vaild for rpm
@danmyway
Copy link
Member

/packit retest-failed

1 similar comment
@danmyway
Copy link
Member

/packit retest-failed

danmyway
danmyway approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@danmyway danmyway left a comment

Choose a reason for hiding this comment

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

PR has been previously approved by @abadger, @bocekm and @SpyTec
No issues or test fails after rebasing to upstream
Great job, @Andrew-ang9 !

@danmyway danmyway merged commit f9e8b49 into oamg:main Jul 13, 2022
@Venefilyn Venefilyn changed the title Stop if the booted kernel is not owned by any package [RHELC-126] Stop if the booted kernel is not owned by any package Jul 22, 2022
@bocekm bocekm mentioned this pull request Aug 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r0x0d r0x0d r0x0d left review comments

@danmyway danmyway danmyway approved these changes

@abadger abadger abadger left review comments

@Venefilyn Venefilyn Venefilyn left review comments

@bocekm bocekm bocekm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Andrew-ang9 @abadger @danmyway @bocekm @r0x0d @Venefilyn