fixes #21 - add OS major/minor/name to yupana #333

patilsuraj767 · 2020-11-19T04:07:58Z

No description provided.

ShimShtein

I suppose it would be easier to change the original regex and parse the OS name and major - minor parts from there directly.

yupana/processor/report_slice_processor.py

kgaikwad

@patilsuraj767,
As @ShimShtein mentioned, you can make use of an existing os-release regex.
Please verify & test below regex if that is helpful to you to add suggested changes related major/minor versions:

re.compile(r'(?P<name>[a-zA-Z\s]*)?\s*((?P<major>\d+)(\.(?P<minor>\d+)(\.(?P<patch>\d+))?)?)\s*(\((?P<code>\S*)\))?')

codecov-io · 2020-12-14T16:10:08Z

Codecov Report

Merging #333 (115b3f4) into master (6594011) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   94.04%   94.06%   +0.02%     
==========================================
  Files          20       20              
  Lines        1393     1399       +6     
  Branches      158      160       +2     
==========================================
+ Hits         1310     1316       +6     
  Misses         58       58              
  Partials       25       25

Impacted Files	Coverage Δ
yupana/processor/report_slice_processor.py	`94.71% <100.00%> (+0.15%)`	⬆️

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 6594011...115b3f4. Read the comment docs.

kgaikwad

@patilsuraj767, added few inline comments.

yupana/processor/report_slice_processor.py

kgaikwad · 2020-12-15T11:58:11Z

yupana/processor/report_slice_processor.py

@@ -254,7 +254,7 @@ def _transform_os_release(self, host: dict):
            return host

        os_version = self._match_regex_and_find_version(os_release)


Would it be good to rename _match_regex_and_find_version method as it is returning os details?

yupana/processor/report_slice_processor.py

kgaikwad · 2020-12-21T08:08:53Z

yupana/processor/report_slice_processor.py

@@ -229,22 +229,28 @@ def _remove_empty_mac_addresses(self, host: dict):
            ))
        return host

-    def _match_regex_and_find_version(self, os_release):
+    def _match_regex_and_find_os_details(self, os_release):
        """Match Regex with os_release and return os_version."""


Could you update function description as well?

kgaikwad · 2020-12-21T10:47:03Z

yupana/processor/report_slice_processor.py

+        host['system_profile']['operating_system'] = {}
+        host['system_profile']['operating_system']['major'] = os_details['major']
+        host['system_profile']['operating_system']['minor'] = os_details['minor']
+
+        if 'Red Hat' in os_details['name']:
+            host['system_profile']['operating_system']['name'] = 'RHEL'
+        else:
+            host['system_profile']['operating_system']['name'] = os_details['name'].strip()


Suggested change

host['system_profile']['operating_system'] = {}

host['system_profile']['operating_system']['major'] = os_details['major']

host['system_profile']['operating_system']['minor'] = os_details['minor']

if 'Red Hat' in os_details['name']:

host['system_profile']['operating_system']['name'] = 'RHEL'

else:

host['system_profile']['operating_system']['name'] = os_details['name'].strip()

host['system_profile']['operating_system'] = {

'major': os_details['major'],

'minor': os_details['minor'],

'name': ('RHEL' if 'Red Hat' in os_details['name'] else os_details['name']).strip() }

ShimShtein · 2020-12-22T14:07:54Z

yupana/processor/tests_report_slice_processor.py

@@ -649,7 +653,8 @@ def test_transform_os_release_when_non_rhel_os(self):
        """Test transform host os_release when non rhel."""
        host = {'system_profile': {'os_release': 'CentOS Linux 7 (Core)'}}
        host = self.processor._transform_single_host(host)
-        self.assertEqual(host, {'system_profile': {'os_release': '7'}})
+        self.assertEqual(host, {'system_profile': {'operating_system': {
+            'major': '7', 'minor': '0', 'name': 'CentOS Linux'}, 'os_release': '7'}})


I think according to the current schema this value of the name field will not be accepted: https://github.com/RedHatInsights/inventory-schemas/blob/5b2fad998ca46b2a5f8e7e314d8cc0577487b581/schemas/system_profile/v1.yaml#L200

Yes, you are right. maxLength is 4.
Any suggestion regarding what should be the name for Non-RHEL OS?

Since it's defined an enum with a single value RHEL, I think the field should be omitted, if it's a non-rhel host.

@patilsuraj767,
In case of non-rhel hosts, could you just omit name field from operating_system and not the whole operating_system key.
@ShimShtein, please correct me in case we will need to omit the operating_system key itself.

Agree with Kavita. I think omitting only name would be better in this case. As we discussed on IRC.

kgaikwad

Thank you @patilsuraj767.
Tested the changes. Works as expected.
LGTM 👍

@ShimShtein, do you have any additional comments?

kgaikwad · 2020-12-29T10:11:12Z

In the schema major and minor defined as integers, after passing string values (example - "7", "8") for major/minor getting a result through host API. That's why as per offline discussion, merging this PR.

kgaikwad · 2020-12-29T10:12:22Z

Thank you @patilsuraj767 and @ShimShtein!

patilsuraj767 requested review from kgaikwad and apuntamb November 19, 2020 11:11

ShimShtein suggested changes Nov 19, 2020

View reviewed changes

yupana/processor/report_slice_processor.py Outdated Show resolved Hide resolved

kgaikwad suggested changes Dec 14, 2020

View reviewed changes

patilsuraj767 added 2 commits December 15, 2020 16:06

fixes quipucords#21 - add OS major/minor/name to yupana

b5b4cf2

Refs quipucords#21 Modified Regex pattern

56df138

patilsuraj767 force-pushed the YUPANA-21-add-OS-major-minor branch from acb8d7b to 56df138 Compare December 15, 2020 10:54

kgaikwad suggested changes Dec 16, 2020

View reviewed changes

Refs quipucords#21 Renamed method to match_regex_and_find_os_details

673075f

kgaikwad suggested changes Dec 21, 2020

View reviewed changes

Refs quipucords#21 Modified doc string

1145ba5

ShimShtein suggested changes Dec 22, 2020

View reviewed changes

Refs quipucords#21 omit 'name' for non-rhel os

115b3f4

kgaikwad approved these changes Dec 29, 2020

View reviewed changes

kgaikwad merged commit 0104dd0 into quipucords:master Dec 29, 2020

kgaikwad mentioned this pull request Dec 29, 2020

Release 0.2.7 of Yupana #337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #21 - add OS major/minor/name to yupana #333

fixes #21 - add OS major/minor/name to yupana #333

patilsuraj767 commented Nov 19, 2020

ShimShtein left a comment

kgaikwad left a comment

codecov-io commented Dec 14, 2020 •

edited

Loading

kgaikwad left a comment

kgaikwad Dec 15, 2020

kgaikwad Dec 21, 2020

kgaikwad Dec 21, 2020

ShimShtein Dec 22, 2020

patilsuraj767 Dec 23, 2020

ShimShtein Dec 23, 2020

kgaikwad Dec 28, 2020

ShimShtein Dec 28, 2020

kgaikwad left a comment

kgaikwad commented Dec 29, 2020

kgaikwad commented Dec 29, 2020

		@@ -254,7 +254,7 @@ def _transform_os_release(self, host: dict):
		return host

		os_version = self._match_regex_and_find_version(os_release)

fixes #21 - add OS major/minor/name to yupana #333

fixes #21 - add OS major/minor/name to yupana #333

Conversation

patilsuraj767 commented Nov 19, 2020

ShimShtein left a comment

Choose a reason for hiding this comment

kgaikwad left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 14, 2020 • edited Loading

Codecov Report

kgaikwad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad left a comment

Choose a reason for hiding this comment

kgaikwad commented Dec 29, 2020

kgaikwad commented Dec 29, 2020

codecov-io commented Dec 14, 2020 •

edited

Loading