Refactor osdetect for rhel family OSes#464
Conversation
fabi200123
force-pushed
the
refactor-osdetect
branch
from
bfd6e77 to
a4ee503
Compare
| } | ||
|
|
||
|
|
||
| def detect_os_from_os_release(os_release, *, match_ids): |
There was a problem hiding this comment.
Let's use type hints and document the "os_release" parameter, stating that it's a dict populated by /etc/os-release values.
| # Copyright 2026 Cloudbase Solutions Srl | ||
| # All Rights Reserved. | ||
|
|
||
| """Shared /etc/os-release parsing for RHEL-family osdetect tools.""" |
There was a problem hiding this comment.
I'd rename the module to "redhat_common.py"
| ``match_ids={"rocky"}`` or ``match_ids={"centos", "almalinux"}``. | ||
| """ | ||
| if not os_release: | ||
| return {} |
There was a problem hiding this comment.
At the very least, we should log a warning if we haven't received the os_release dict.
|
|
||
| version = os_release.get("VERSION_ID") | ||
| if not version: | ||
| return {} |
There was a problem hiding this comment.
Same here, the OS id matches but it doesn't include a version. Let's log a warning.
|
|
||
| default_name = OS_RELEASE_ID_MAP.get(os_id) | ||
| if not default_name: | ||
| return {} |
There was a problem hiding this comment.
Warning? The id matches but we don't have a default distribution name for that given identifier.
| return {} | ||
|
|
||
| name = os_release.get("NAME") or default_name | ||
| if os_id == "centos" and "Stream" in name: |
There was a problem hiding this comment.
Do we really need this special check for centos stream? I tried out a docker image and got NAME="CentOS Stream", which already matches CENTOS_STREAM_DISTRO_IDENTIFIER
Speaking of which, won't there always be a NAME entry? Do we actually need a dict of distro names?
There was a problem hiding this comment.
The Stream was added starting with CentOS 8, regarding the NAME entry, I agree on using NAME, this was the previous name that was used (didn't want to diverge too much)
There was a problem hiding this comment.
My point is that the default will probably never be used, making OS_RELEASE_ID_MAP redundant.
| distribution_name, version)} | ||
| return info | ||
| return common.detect_os_from_os_release( | ||
| self._get_os_release(), |
There was a problem hiding this comment.
RHEL 6 and derivate OS-es do not have the /etc/os-release file. Are we going to drop RHEL 6 support? If so, the PR description must clearly state it. We can also remove RHEL 6 specific os-morphing code.
2 participants
This PR refactors the osdetect for the OSes that are RHEL related. It adds a
common.pywhere thedetect_os_from_os_releaseis defined to return the proper info from/etc/os-release:It keeps the previous format for
distribution_namebased onDISTRO_IDENTIFIER.