Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fetching grains['os'] from /etc/os-release on SUSE systems if it is possible #33903

Merged
merged 8 commits into from
Jun 16, 2016
Merged

Fetching grains['os'] from /etc/os-release on SUSE systems if it is possible #33903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c00c9b5
Adding SLES_SAP to OS_FAMILY_MAP
meaksh Jun 9, 2016
7603139
Getting the 'os' grain from CPE_NAME inside /etc/os-release for SUSE …
meaksh Jun 9, 2016
7f7800f
testing if SUSE os grain is set from /etc/os-release
meaksh Jun 9, 2016
63e2f27
lint fix
meaksh Jun 9, 2016
1680f18
osarch mock for unit test
meaksh Jun 10, 2016
a0693aa
fix: osarch_mock
meaksh Jun 10, 2016
1afc12f
Test fixed: get OS grain from /etc/os-release if possible
meaksh Jun 13, 2016
50175a0
one clause to set OS grain from CPE_NAME
meaksh Jun 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion salt/grains/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,7 @@ def id_():
'SUSE': 'SUSE',
'openSUSE Leap': 'SUSE',
'openSUSE Tumbleweed': 'SUSE',
'SLES_SAP': 'SUSE',
'Solaris': 'Solaris',
'SmartOS': 'Solaris',
'OpenIndiana Development': 'Solaris',
Expand Down Expand Up @@ -1283,6 +1284,9 @@ def os_data():
grains['lsb_distrib_release'] = os_release['VERSION_ID']
if 'PRETTY_NAME' in os_release:
grains['lsb_distrib_codename'] = os_release['PRETTY_NAME']
if 'CPE_NAME' in os_release:
if ":suse:" in os_release['CPE_NAME'] or ":opensuse:" in os_release['CPE_NAME']:
grains['os'] = "SUSE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

elif os.path.isfile('/etc/SuSE-release'):
grains['lsb_distrib_id'] = 'SUSE'
version = ''
Expand Down Expand Up @@ -1390,7 +1394,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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@isbm isbm Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

grains['os'] = _OS_NAME_MAP.get(shortname, distroname)
grains.update(_linux_cpudata())
grains.update(_linux_gpu_data())
elif grains['kernel'] == 'SunOS':
Expand Down
66 changes: 66 additions & 0 deletions tests/unit/grains/core_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,72 @@ def _import_mock(name, *args):

self.assertEqual(os_grains.get('os_family'), 'Debian')

@skipIf(not salt.utils.is_linux(), 'System is not Linux')
def test_suse_os_from_cpe_data(self):
'''
Test if 'os' grain is parsed from CPE_NAME of /etc/os-release
'''
_path_exists_map = {
'/proc/1/cmdline': False
}
_path_isfile_map = {
'/etc/os-release': True,
}
_os_release_map = {
'NAME': 'SLES',
'VERSION': '12-SP1',
'VERSION_ID': '12.1',
'PRETTY_NAME': 'SUSE Linux Enterprise Server 12 SP1',
'ID': 'sles',
'ANSI_COLOR': '0;32',
'CPE_NAME': 'cpe:/o:suse:sles:12:sp1'
}

path_exists_mock = MagicMock(side_effect=lambda x: _path_exists_map[x])
path_isfile_mock = MagicMock(
side_effect=lambda x: _path_isfile_map.get(x, False)
)
empty_mock = MagicMock(return_value={})
osarch_mock = MagicMock(return_value="amd64")
os_release_mock = MagicMock(return_value=_os_release_map)

orig_import = __import__

def _import_mock(name, *args):
if name == 'lsb_release':
raise ImportError('No module named lsb_release')
return orig_import(name, *args)

# Skip the first if statement
with patch.object(salt.utils, 'is_proxy',
MagicMock(return_value=False)):
# Skip the selinux/systemd stuff (not pertinent)
with patch.object(core, '_linux_bin_exists',
MagicMock(return_value=False)):
# Skip the init grain compilation (not pertinent)
with patch.object(os.path, 'exists', path_exists_mock):
# Ensure that lsb_release fails to import
with patch('__builtin__.__import__',
side_effect=_import_mock):
# Skip all the /etc/*-release stuff (not pertinent)
with patch.object(os.path, 'isfile', path_isfile_mock):
with patch.object(core, '_parse_os_release', os_release_mock):
# Mock platform.linux_distribution to give us the
# OS name that we want.
distro_mock = MagicMock(
return_value=('SUSE Linux Enterprise Server ', '12', 'x86_64')
)
with patch.object(platform, 'linux_distribution', distro_mock):
with patch.object(core, '_linux_gpu_data', empty_mock):
with patch.object(core, '_linux_cpudata', empty_mock):
with patch.object(core, '_virtual', empty_mock):
# Mock the osarch
with patch.dict(core.__salt__, {'cmd.run': osarch_mock}):
os_grains = core.os_data()

self.assertEqual(os_grains.get('os_family'), 'SUSE')
self.assertEqual(os_grains.get('os'), 'SUSE')


if __name__ == '__main__':
from integration import run_tests
Expand Down