[RHELC-1427] Fix rpm -Va parsing and improve speed #1319

hosekadam · 2024-07-24T13:45:32Z

Fix rpm -Va parsing to accept also ghosts files. Do not generate message
about invalid output of lines related to ghosts files anymore. Those files
aren't backed up as they are temporary.

Extend rpm -Va command by --nodeps switch. This improves
speed of the command and doesn't generate unimportant info about
unsatisfied dependencies for packages.

Jira Issues:

RHELC-1427

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

has-bot · 2024-07-24T13:46:07Z

/packit test --labels sanity

Comment generated by an automation.

Log | Bot Usage

codecov · 2024-07-24T13:48:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.18%. Comparing base (3ee8b8f) to head (265ccc5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1319   +/-   ##
=======================================
  Coverage   96.18%   96.18%           
=======================================
  Files          58       58           
  Lines        4826     4828    +2     
  Branches      847      848    +1     
=======================================
+ Hits         4642     4644    +2     
  Misses        106      106           
  Partials       78       78

Flag	Coverage Δ
centos-linux-7	`91.45% <100.00%> (+<0.01%)`	⬆️
centos-linux-8	`92.36% <100.00%> (+<0.01%)`	⬆️
centos-linux-9	`92.41% <100.00%> (+<0.01%)`	⬆️

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

LGTM! But just to lean more on comments and commit messages.

In the commit it says

Fix rpm -Va parsing when backing up package files to accept also ghost
files. Those files aren't backed up, but message about invalid output
was removed by this.

I'd change this around to something along the lines of "Fix invalid output in rpm -Va due to ghost files being incorrectly parsed. Ghost files are not needed by the tool as they relate to x and can be safely skipped" or something in that regard. At least to put the reason for the fix in the beginning and not at the end of the paragraph

Improve speed of rpm -Va command by using --nodeps switch. This improves
speed of the command by 40 % and doesn't generate unimportant info about
unsatisfied dependencies for packages.

If you have a statistic of 40% can you add that to the message. 40% is also very inconvenient by itself. For example, if something took 1.0s and was improved to now only take 0.1s overall, that is a 90% performance increase, but overall you would never notice

Venefilyn · 2024-07-26T14:45:23Z

convert2rhel/systeminfo.py

@@ -390,7 +390,7 @@ def generate_rpm_va(self, log_filename=PRE_RPM_VA_LOG_FILENAME):
            " minutes. It can be disabled by using the"
            " --no-rpm-va option."
        )
-        rpm_va, _ = utils.run_subprocess(["rpm", "-Va"], print_output=False)
+        rpm_va, _ = utils.run_subprocess(["rpm", "-Va", "--nodeps"], print_output=False)


What does --nodeps do? Add a comment here

Venefilyn · 2024-07-26T14:46:08Z

convert2rhel/actions/pre_ponr_changes/backup_system.py

+            # Skip ghost files RHELC-1427
+            if file["file_type"] == "g":
+                continue


Why are we skipping? Comment explaining why. Imagine the person reading doesn't know what ghost files are used for or why we don't care about it

hosekadam · 2024-07-31T09:14:56Z

@Venefilyn I've improved the comments and commit description. Do you like those changes?

Venefilyn

LGTM! Just a small nitpick on the commit message but not a blocker, can be merged

Improve speed of rpm -Va command by using --nodeps switch. This improves
speed of the command and doesn't generate unimportant info about
unsatisfied dependencies for packages.

Improves speed is mentioned twice here

Venefilyn · 2024-07-31T13:02:58Z

/packit test --labels sanity

convert2rhel/systeminfo.py

Venefilyn · 2024-08-05T11:57:59Z

/packit test --labels sanity

Venefilyn · 2024-08-06T10:47:33Z

Tests failing due to this:

convert2rhel = <function convert2rhel.<locals>.factory at 0x7fcb34170840>

    def test_analyze_no_rpm_va_option(convert2rhel):
        """
        This test verifies a basic incompatibility of the analyze and --no-rpm-va options.
        The user should be warned that the --no-rpm-va option will be ignored and the command
        will be called.
        """
        with convert2rhel("analyze -y --no-rpm-va --debug") as c2r:
            c2r.expect("We will proceed with ignoring the --no-rpm-va option")
>           c2r.expect_exact("Calling command 'rpm -Va'")

test_basic_sanity_checks.py:297:

hosekadam · 2024-08-07T09:34:22Z

/packit test --labels sanity

Fix rpm -Va parsing to accept also ghosts files. Do not generate message about invalid output of lines related to ghosts files anymore. Those files aren't backed up as they are temporary. Extend rpm -Va command by --nodeps switch. This improves speed of the command and doesn't generate unimportant info about unsatisfied dependencies for packages.

hosekadam added kind/bug-fix A bug has been fixed tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. labels Jul 24, 2024

hosekadam force-pushed the fix-rpm-va-parsing branch from 1188693 to 462e786 Compare July 25, 2024 14:58

hosekadam requested a review from Venefilyn July 26, 2024 08:02

Venefilyn reviewed Jul 26, 2024

View reviewed changes

hosekadam force-pushed the fix-rpm-va-parsing branch from 462e786 to 622ea9d Compare July 31, 2024 09:14

Venefilyn approved these changes Jul 31, 2024

View reviewed changes

Venefilyn self-assigned this Jul 31, 2024

r0x0d reviewed Aug 2, 2024

View reviewed changes

convert2rhel/systeminfo.py Outdated Show resolved Hide resolved

hosekadam force-pushed the fix-rpm-va-parsing branch from 622ea9d to 620a03d Compare August 7, 2024 09:20

hosekadam requested review from danmyway, kokesak, SerCantus and a team as code owners August 7, 2024 09:20

hosekadam force-pushed the fix-rpm-va-parsing branch from 620a03d to 3e437cb Compare August 7, 2024 09:20

kokesak approved these changes Aug 7, 2024

View reviewed changes

Venefilyn force-pushed the fix-rpm-va-parsing branch from 3e437cb to 265ccc5 Compare August 12, 2024 14:37

Venefilyn added the merge-after-tests-ok label Aug 12, 2024

hosekadam merged commit 4defcdc into oamg:main Aug 12, 2024
21 checks passed

This was referenced Aug 21, 2024

Bump to version 2.1.0 #1352

Merged

Bump to version 2.1.0 #1356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1427] Fix rpm -Va parsing and improve speed #1319

[RHELC-1427] Fix rpm -Va parsing and improve speed #1319

hosekadam commented Jul 24, 2024 •

edited

Loading

has-bot commented Jul 24, 2024

codecov bot commented Jul 24, 2024 •

edited

Loading

Venefilyn left a comment

Venefilyn Jul 26, 2024

Venefilyn Jul 26, 2024 •

edited

Loading

hosekadam commented Jul 31, 2024 •

edited

Loading

Venefilyn left a comment

Venefilyn commented Jul 31, 2024

Venefilyn commented Aug 5, 2024

Venefilyn commented Aug 6, 2024

hosekadam commented Aug 7, 2024

[RHELC-1427] Fix rpm -Va parsing and improve speed #1319

[RHELC-1427] Fix rpm -Va parsing and improve speed #1319

Conversation

hosekadam commented Jul 24, 2024 • edited Loading

has-bot commented Jul 24, 2024

codecov bot commented Jul 24, 2024 • edited Loading

Codecov Report

Venefilyn left a comment

Choose a reason for hiding this comment

Venefilyn Jul 26, 2024

Choose a reason for hiding this comment

Venefilyn Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

hosekadam commented Jul 31, 2024 • edited Loading

Venefilyn left a comment

Choose a reason for hiding this comment

Venefilyn commented Jul 31, 2024

Venefilyn commented Aug 5, 2024

Venefilyn commented Aug 6, 2024

hosekadam commented Aug 7, 2024

hosekadam commented Jul 24, 2024 •

edited

Loading

codecov bot commented Jul 24, 2024 •

edited

Loading

Venefilyn Jul 26, 2024 •

edited

Loading

hosekadam commented Jul 31, 2024 •

edited

Loading