Fetching grains['os'] from /etc/os-release on SUSE systems if it is possible #33903
Conversation
|
Lint wants less lines. |
|
The new test is failing in some environments. I have to fix it. |
|
Still doesn't look like we're quite there with this failing test. Please let me know when this is ready, @meaksh and thanks! |
|
Hi @cachedout , the test is now fixed and working. Sorry for the delay :) |
| @@ -1390,7 +1396,8 @@ def os_data(): | |||
| shortname = distroname.replace(' ', '').lower()[:10] | |||
| # this maps the long names from the /etc/DISTRO-release files to the | |||
| # traditional short names that Salt has used. | |||
| grains['os'] = _OS_NAME_MAP.get(shortname, distroname) | |||
| if 'os' not in grains: | |||
There was a problem hiding this comment.
The SUSE-specific stuff doesn't concern me but this bit affects all distros. Could you clarify your reasoning here? I tend to be very cautious about modifying anything that touches global grains detection logic. We have been bitten by this many times.
There was a problem hiding this comment.
@cachedout I don't see a problem here. The 'os' key was always set from the keymap, but in our systems it is not always same. Hence @meaksh first trying to modify grain before picking standard key. In case not found anything, one from _OS_NAME_MAP is taken. I would say 👍 here, actually.
However, I see another trouble 💥 here: the whole code of this particular function soon to be a better "winner" than a Toyota design. I mean, let's look at this monster function... It is horrid and needs to be splitted, refactored and cleaned up. IMHO. Yes, only in develop, no worries. 😉
Just my few cents.
There was a problem hiding this comment.
@cachedout I understand your concern, but currently I think there is no problem here. Before this PR, grains['os'] in linux systems is always picked from _OS_NAME_MAP and there isn't custom assignments for this grain in this scope.
Now, with the PR, the only case where grains['os'] is set manually is our SUSE cases and we don't want to overwrite it later with the default value of _OS_NAME_MAP. So the default behavior for the os grain assignment will be the same that currently is except for SUSE systems.
What do you think?
Thanks @isbm for the comment
cachedout
added
the
Pending-Discussion
| if ":suse:" in os_release['CPE_NAME']: | ||
| grains['os'] = "SUSE" | ||
| elif ":opensuse:" in os_release['CPE_NAME']: | ||
| grains['os'] = "SUSE" |
There was a problem hiding this comment.
It is too much noise for setting the os key. I would put that into one clause:
if ":suse:" in os_release['CPE_NAME'] or ":opensuse:" in os_release['CPE_NAME']:
grains['os'] = "SUSE"
|
@cachedout from my side here is 👍 There are some more fixes coming this regard (Leap/Tumbleweed), but this PR should be closed first. |
What does this PR do?
This PR fixes a bug solved for one of our customers which uses
SUSE SLES SAPsystems. It also defines a new rule to get thegrains['os']in SUSE family systems.Currently, Salt uses the
_OS_FAMILY_MAPto get thegrains['os_family']based ongrains['os']grain, but it implies that we have to keep the map updated for all the possibleosnamefor SUSE products.So we propose to try to use the
CPE_NAMEinformation contained in/etc/os-releasein order to know if it's a SUSE product and also to set thegrains['os'] == "SUSE"Tests written?
Yes
/cc @isbm