[RHELC-676] Unify parsing of the system release string #897

bookwar · 2023-08-31T17:42:06Z

There are several places where the system release string is parsed via regular expressions. These regexps are different and adjusting them requires refactoring in multiple places.

This change introduces one parsing function which does a full match of the system-release string and reads all possible data points from it, and provides the result as a dictionary for later use by other tools.

The function replaces three other helper methods: _get_system_name, _get_system_version and _get_system_distribution_id, which are removed.

NOTE1: The parsing function could be a static method or even implemented as a utility function outside the class. The only reason why it is in the class is the logger object.

NOTE2: the function overlaps with the get_system_release_info function. We should check if this needs to be further unified.

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

has-bot · 2023-08-31T19:39:43Z

/packit test --labels tier0

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

Log | Bot Usage

abadger

Nice restructuring. Much appreciated. Just had a few comments on the Draft.

convert2rhel/systeminfo.py

has-bot · 2024-02-26T10:15:56Z

/packit test --labels sanity

Comment generated by an automation.

Log | Bot Usage

codecov · 2024-03-07T19:43:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.26%. Comparing base (28908cd) to head (736b7ec).
Report is 3 commits behind head on main.

❗ Current head 736b7ec differs from pull request most recent head 1cbf6e3. Consider uploading reports for the commit 1cbf6e3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
- Coverage   95.35%   95.26%   -0.09%     
==========================================
  Files          51       51              
  Lines        4605     4604       -1     
  Branches      810      810              
==========================================
- Hits         4391     4386       -5     
- Misses        137      140       +3     
- Partials       77       78       +1

Flag	Coverage Δ
centos-linux-7	`90.33% <100.00%> (-0.09%)`	⬇️
centos-linux-8	`91.31% <100.00%> (-0.09%)`	⬇️
centos-linux-9	`91.37% <100.00%> (-0.09%)`	⬇️

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.

bookwar · 2024-03-15T18:06:51Z

/packit test --labels sanity

convert2rhel/unit_tests/systeminfo_test.py

convert2rhel/systeminfo.py

r0x0d · 2024-04-05T13:27:56Z

convert2rhel/systeminfo.py

        if not match:
-            self.logger.critical("Couldn't get system version from the content string: %s" % content)
-        version = Version(int(match.group(1)), int(match.group(2)))
+            self.logger.critical_no_exit("Couldn't parse the system release content string: %s" % content)


Why do we changed this to no_exit? It seems to me very critical to inform that the system release file is not correct.

Also, I think we will raise an exception while resolving system info, right? Is it fine if we just bubble up the exception?

[2024-04-05T13:49:33+0000] TASK - [Prepare: Gather system information] ******************************* CRITICAL - Couldn't parse the system release content string: CentOS Linux release Writing breadcrumbs to '/etc/migration-results'. [2024-04-05T13:49:33+0000] DEBUG - No RHSM facts folder found at '/etc/rhsm/facts'. Creating a new one... Writing RHSM custom facts to '/etc/rhsm/facts/convert2rhel.facts'. [2024-04-05T13:49:33+0000] DEBUG - Traceback (most recent call last): File "/usr/lib/python3.6/site-packages/convert2rhel/main.py", line 108, in main_locked gather_system_info() File "/usr/lib/python3.6/site-packages/convert2rhel/main.py", line 263, in gather_system_info systeminfo.system_info.resolve_system_info() File "/usr/lib/python3.6/site-packages/convert2rhel/systeminfo.py", line 122, in resolve_system_info self.name = system_release_data["name"] KeyError: 'name' No changes were made to the system. [2024-04-05T13:49:33+0000] DEBUG - /var/run/lock/convert2rhel.pid PID 34285 unlocked. make: *** [Makefile:25: analyze] Error 1

My intent is that parse_.. function is a simple utility function. It should not make decisions on should we stop the execution or not. If parses what it can find, and then the higher-level structure may decide what to do with this information. In this case it still fails, and that's expected.

It maybe eventually we would be overriding these parameters from a config file or whatever.

I could have raise a custom exception here instead of passing None as a reply. But it seems like an overkill.

This wasn't supposed to be an exit from the start. I just didn't realize at the beginning that logger.critical() does the exit. And didn't cover that scenario in a unit test. Then while I was adding a unit test for the failure scenario, I saw that the function does exit() and that's not what I wanted.

Alright, got the idea behind... And I agree that parse_* functions should all have that same intention, but at the same time, I feel that we are making some type of "regression". Right now, when this fails, it will show to the user a red message with the exact error, making it less scary if something goes wrong.

With this change, we are going away from a message that seems informative about the error, and just raising an base exception that, sometimes, may not be useful to the user.

What if we handle this type of exception in a higher level function, just so we can maintain the same UX for the user, without them noticing that we changed how we parse the content string?

convert2rhel/unit_tests/systeminfo_test.py

There are several places where the system release string is parsed via regular expression. These expressions are different and adjusting them requires refactoring in multiple places. This change introduces one parsing function which does a full match of the system-release string and reads all possible data points from it, and provides the result as a dictionary for later use by other tools. The function replaces three other helper methods: _get_system_name, _get_system_version and _get_system_distribution_id, which are removed. NOTE1: The parsing function could be a static method or even implemented as a utility function outside the class. The only reason why it is in the class is the logger object. NOTE2: the function overlaps with the get_system_release_info function. We should check if this needs to be further unified.

r0x0d

Looks good to me.

Only one comment about critical vs critical_no_exit, but other than that, couldn't find anything that would block the PR for me.

Co-authored-by: Freya Gustavsson <freya@spytec.se>

Venefilyn · 2024-04-15T12:15:29Z

/packit test --labels sanity

Venefilyn

Integration test failures seem unrelated, merging

abadger added the tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Aug 31, 2023

bookwar force-pushed the system-release-content branch 3 times, most recently from f8a7c85 to 399f16f Compare August 31, 2023 22:51

abadger reviewed Sep 14, 2023

View reviewed changes

convert2rhel/systeminfo.py Show resolved Hide resolved

convert2rhel/systeminfo.py Show resolved Hide resolved

Venefilyn marked this pull request as draft September 19, 2023 14:59

Venefilyn changed the title ~~[Draft] Unify parsing of the system release string~~ Unify parsing of the system release string Sep 19, 2023

bookwar force-pushed the system-release-content branch 2 times, most recently from e7ffce9 to ae0101a Compare September 26, 2023 11:43

Venefilyn mentioned this pull request Oct 2, 2023

[RHELC-880] Add --eus option to enable RHEL 8 EUS repositories #947

Merged

8 tasks

bookwar force-pushed the system-release-content branch from 5c13c18 to d0ed5e4 Compare February 26, 2024 09:44

bookwar added tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. and removed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Feb 26, 2024

bookwar force-pushed the system-release-content branch 3 times, most recently from 40e5846 to 44e3c62 Compare March 7, 2024 19:42

bookwar changed the title ~~Unify parsing of the system release string~~ [RHELC-676] Unify parsing of the system release string Mar 13, 2024

bookwar added the kind/feature New feature or request label Mar 14, 2024

bookwar force-pushed the system-release-content branch from 44e3c62 to 86dbe7c Compare March 14, 2024 18:27

bookwar marked this pull request as ready for review March 14, 2024 18:28

Venefilyn reviewed Mar 18, 2024

View reviewed changes

convert2rhel/unit_tests/systeminfo_test.py Outdated Show resolved Hide resolved

convert2rhel/systeminfo.py Outdated Show resolved Hide resolved

hosekadam reviewed Mar 20, 2024

View reviewed changes

convert2rhel/systeminfo.py Show resolved Hide resolved

bookwar force-pushed the system-release-content branch 3 times, most recently from 962f276 to 81dd2e5 Compare March 21, 2024 15:35

Venefilyn reviewed Mar 21, 2024

View reviewed changes

convert2rhel/systeminfo.py Show resolved Hide resolved

convert2rhel/systeminfo.py Outdated Show resolved Hide resolved

r0x0d reviewed Apr 5, 2024

View reviewed changes

convert2rhel/unit_tests/systeminfo_test.py Show resolved Hide resolved

bookwar added 2 commits April 9, 2024 13:47

Rename match to matched to avoid name collision

736b7ec

bookwar force-pushed the system-release-content branch from 81dd2e5 to 736b7ec Compare April 9, 2024 13:55

r0x0d reviewed Apr 10, 2024

View reviewed changes

Ignote type hint on re.match

1cbf6e3

Co-authored-by: Freya Gustavsson <freya@spytec.se>

Venefilyn approved these changes Apr 15, 2024

View reviewed changes

Venefilyn merged commit c49cd04 into oamg:main Apr 15, 2024
18 of 25 checks passed

hosekadam mentioned this pull request May 27, 2024

Bump version to 2.0.0 #1243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-676] Unify parsing of the system release string #897

[RHELC-676] Unify parsing of the system release string #897

bookwar commented Aug 31, 2023 •

edited

Loading

has-bot commented Aug 31, 2023

abadger left a comment

has-bot commented Feb 26, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

bookwar commented Mar 15, 2024

r0x0d Apr 5, 2024

r0x0d Apr 5, 2024

bookwar Apr 9, 2024 •

edited

Loading

r0x0d Apr 10, 2024

r0x0d left a comment

Venefilyn commented Apr 15, 2024

Venefilyn left a comment

[RHELC-676] Unify parsing of the system release string #897

[RHELC-676] Unify parsing of the system release string #897

Conversation

bookwar commented Aug 31, 2023 • edited Loading

has-bot commented Aug 31, 2023

abadger left a comment

Choose a reason for hiding this comment

has-bot commented Feb 26, 2024

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

bookwar commented Mar 15, 2024

r0x0d Apr 5, 2024

Choose a reason for hiding this comment

r0x0d Apr 5, 2024

Choose a reason for hiding this comment

bookwar Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

r0x0d Apr 10, 2024

Choose a reason for hiding this comment

r0x0d left a comment

Choose a reason for hiding this comment

Venefilyn commented Apr 15, 2024

Venefilyn left a comment

Choose a reason for hiding this comment

bookwar commented Aug 31, 2023 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

bookwar Apr 9, 2024 •

edited

Loading