-
Notifications
You must be signed in to change notification settings - Fork 210
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.
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
Improve parsers/redhat_release to detect CentOS correctly #3832
base: master
Are you sure you want to change the base?
Conversation
The RedhatRelease parser already does a basic detection of RHEL, CentOS, Fedora or Unknown operating system. We need to test this with the RedHatRelease combiner. Part of this work simplifies the short product code detection in the RedhatRelease parser, which we now advertise as a property. Signed-off-by: Paul Wayper <paulway@redhat.com>
Signed-off-by: Paul Wayper <paulway@redhat.com>
Signed-off-by: Paul Wayper <paulway@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PaulWay - This change looks good to me, but the update of the combiner will break some tests of the GSS-Rules, since these tests take the RedHatRelease
for RHEL only. We'd better fix them before merging this PR.
Good point @xiangce - I'll look through the rules and write up a PR for that. |
@@ -74,7 +74,7 @@ def __init__(self, uname, rh_rel): | |||
self.major = uname.redhat_release.major | |||
self.minor = uname.redhat_release.minor | |||
self.rhel = '{0}.{1}'.format(self.major, self.minor) | |||
elif rh_rel and rh_rel.is_rhel: | |||
elif rh_rel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PaulWay - Other than making an MR to the GSS rules, it would be more suitable to keep this RedHatRelease
combiner for RHEL only, As this combiner is designed for RHEL system only, See the docstring in L40
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Xiangce, I wanted to say something about this previously but forgot. I feel like detection for centos, fedora, oracle, etc should be done in the os-release parser not the redha-release parser/combiner. Since os-release is used by every distro, and provides a lot of information that the redhat-release file doesn't provide I feel it's better fitted.
Can one of the admins verify this patch? |
All Pull Requests:
Check all that apply:
Complete Description of Additions/Changes:
The RedhatRelease parser already does a basic check of the operating system to set flags
for whether this is a RHEL, CentOS, Fedora or unknown system. We need to test that these
are handled correctly within the RedHatRelease combiner.
Part of this work simplifies the short product code detection in the
RedhatRelease parser, which we now advertise as a property.
Additionally, RHEV 3.5 now correctly detects as a RHEL operating system.