Merge ~chad.smith/cloud-init:bug/1780481-fix-get-linux-distro into cloud-init:master

Proposed by Chad Smith on 2018-07-06
Status: Merged
Approved by: Chad Smith on 2018-07-09
Approved revision: 46345394b6d8ec96f13ade59c9fffb2885349ade
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~chad.smith/cloud-init:bug/1780481-fix-get-linux-distro
Merge into: cloud-init:master
Diff against target: 151 lines (+89/-12)
3 files modified
cloudinit/tests/test_util.py (+68/-1)
cloudinit/util.py (+18/-10)
tests/unittests/test_datasource/test_azure_helper.py (+3/-1)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2018-07-09
Scott Moser 2018-07-06 Approve on 2018-07-09
Review via email: mp+349081@code.launchpad.net

Commit message

ubuntu,centos,debian: get_linux_distro to align with platform.dist

A recent commit added get_linux_distro to replace the deprecated python
platform.dist module behavior before it is dropped from python. It added
behavior that was compliant on OpenSuSE and SLES, by returning
(<distro_name>, <distro_version>, <cpu-arch>).

Fix get_linux_distro to behave more like the specific distribution's
platform.dist on ubuntu, centos and debian, which will return the
distribution release codename as the third element instead of <cpu-arch>.

SLES and OpenSUSE will retain their current behavior.

Examples follow:
('sles', '15', 'x86_64')
('opensuse', '42.3', 'x86_64')
('debian', '9', 'stretch')
('ubuntu', '16.04', 'xenial')
('centos', '7', 'Core')

LP: #1780481

To post a comment you must log in.
Robert Schweikert (rjschwei) wrote :

LGTM +1

FAILED: Continuous integration, rev:2099b4ce71ce2deba68e38b1443746ea0b125b9c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/141/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/141/rebuild

review: Needs Fixing (continuous-integration)
a2f229b... by Chad Smith on 2018-07-07

need to mock /etc/os-release for azure util.is_FreeBSD checks

FAILED: Continuous integration, rev:a2f229bcd228816ff3f2047ae3f3b4fb93fc86ba
https://jenkins.ubuntu.com/server/job/cloud-init-ci/142/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/142/rebuild

review: Needs Fixing (continuous-integration)
b1ea4ac... by Chad Smith on 2018-07-07

not bytes mr smith

PASSED: Continuous integration, rev:b1ea4acedb2a107f62fdcde639640ce2b8b007bc
https://jenkins.ubuntu.com/server/job/cloud-init-ci/143/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/143/rebuild

review: Approve (continuous-integration)
9ae57dd... by Chad Smith on 2018-07-09

shlex instead

Scott Moser (smoser) wrote :

2 comments
a.) it seems wrong to have sles and opensuse reatin their old behavior
b.) can get_linux_distro use load_shell_content ?

FAILED: Continuous integration, rev:9ae57dd91778fd655f62e64e9d2e6adfa44c77dc
https://jenkins.ubuntu.com/server/job/cloud-init-ci/144/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/144/rebuild

review: Needs Fixing (continuous-integration)
a871140... by Chad Smith on 2018-07-09

use load_shell_content per smoser's review

FAILED: Continuous integration, rev:a8711404dc56287eefdffd5e6a88ebc4de4cb250
https://jenkins.ubuntu.com/server/job/cloud-init-ci/145/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/145/rebuild

review: Needs Fixing (continuous-integration)
Scott Moser (smoser) wrote :

looks good to me with a fix of c-i

review: Approve
4634539... by Chad Smith on 2018-07-09

proper indentation of debian /etc/os-release content. mock is_FreeBSD to avoid having to whitebox mock load_file calls

PASSED: Continuous integration, rev:46345394b6d8ec96f13ade59c9fffb2885349ade
https://jenkins.ubuntu.com/server/job/cloud-init-ci/146/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/146/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
2index 17853fc..6a31e50 100644
3--- a/cloudinit/tests/test_util.py
4+++ b/cloudinit/tests/test_util.py
5@@ -26,8 +26,51 @@ OS_RELEASE_SLES = dedent("""\
6 CPE_NAME="cpe:/o:suse:sles:12:sp3"\n
7 """)
8
9+OS_RELEASE_OPENSUSE = dedent("""\
10+NAME="openSUSE Leap"
11+VERSION="42.3"
12+ID=opensuse
13+ID_LIKE="suse"
14+VERSION_ID="42.3"
15+PRETTY_NAME="openSUSE Leap 42.3"
16+ANSI_COLOR="0;32"
17+CPE_NAME="cpe:/o:opensuse:leap:42.3"
18+BUG_REPORT_URL="https://bugs.opensuse.org"
19+HOME_URL="https://www.opensuse.org/"
20+""")
21+
22+OS_RELEASE_CENTOS = dedent("""\
23+ NAME="CentOS Linux"
24+ VERSION="7 (Core)"
25+ ID="centos"
26+ ID_LIKE="rhel fedora"
27+ VERSION_ID="7"
28+ PRETTY_NAME="CentOS Linux 7 (Core)"
29+ ANSI_COLOR="0;31"
30+ CPE_NAME="cpe:/o:centos:centos:7"
31+ HOME_URL="https://www.centos.org/"
32+ BUG_REPORT_URL="https://bugs.centos.org/"
33+
34+ CENTOS_MANTISBT_PROJECT="CentOS-7"
35+ CENTOS_MANTISBT_PROJECT_VERSION="7"
36+ REDHAT_SUPPORT_PRODUCT="centos"
37+ REDHAT_SUPPORT_PRODUCT_VERSION="7"
38+""")
39+
40+OS_RELEASE_DEBIAN = dedent("""\
41+ PRETTY_NAME="Debian GNU/Linux 9 (stretch)"
42+ NAME="Debian GNU/Linux"
43+ VERSION_ID="9"
44+ VERSION="9 (stretch)"
45+ ID=debian
46+ HOME_URL="https://www.debian.org/"
47+ SUPPORT_URL="https://www.debian.org/support"
48+ BUG_REPORT_URL="https://bugs.debian.org/"
49+""")
50+
51 OS_RELEASE_UBUNTU = dedent("""\
52 NAME="Ubuntu"\n
53+ # comment test
54 VERSION="16.04.3 LTS (Xenial Xerus)"\n
55 ID=ubuntu\n
56 ID_LIKE=debian\n
57@@ -310,7 +353,31 @@ class TestGetLinuxDistro(CiTestCase):
58 m_os_release.return_value = OS_RELEASE_UBUNTU
59 m_path_exists.side_effect = TestGetLinuxDistro.os_release_exists
60 dist = util.get_linux_distro()
61- self.assertEqual(('ubuntu', '16.04', platform.machine()), dist)
62+ self.assertEqual(('ubuntu', '16.04', 'xenial'), dist)
63+
64+ @mock.patch('cloudinit.util.load_file')
65+ def test_get_linux_centos(self, m_os_release, m_path_exists):
66+ """Verify we get the correct name and release name on CentOS."""
67+ m_os_release.return_value = OS_RELEASE_CENTOS
68+ m_path_exists.side_effect = TestGetLinuxDistro.os_release_exists
69+ dist = util.get_linux_distro()
70+ self.assertEqual(('centos', '7', 'Core'), dist)
71+
72+ @mock.patch('cloudinit.util.load_file')
73+ def test_get_linux_debian(self, m_os_release, m_path_exists):
74+ """Verify we get the correct name and release name on Debian."""
75+ m_os_release.return_value = OS_RELEASE_DEBIAN
76+ m_path_exists.side_effect = TestGetLinuxDistro.os_release_exists
77+ dist = util.get_linux_distro()
78+ self.assertEqual(('debian', '9', 'stretch'), dist)
79+
80+ @mock.patch('cloudinit.util.load_file')
81+ def test_get_linux_opensuse(self, m_os_release, m_path_exists):
82+ """Verify we get the correct name and machine arch on OpenSUSE."""
83+ m_os_release.return_value = OS_RELEASE_OPENSUSE
84+ m_path_exists.side_effect = TestGetLinuxDistro.os_release_exists
85+ dist = util.get_linux_distro()
86+ self.assertEqual(('opensuse', '42.3', platform.machine()), dist)
87
88 @mock.patch('platform.dist')
89 def test_get_linux_distro_no_data(self, m_platform_dist, m_path_exists):
90diff --git a/cloudinit/util.py b/cloudinit/util.py
91index 6da9511..d0b0e90 100644
92--- a/cloudinit/util.py
93+++ b/cloudinit/util.py
94@@ -579,16 +579,24 @@ def get_cfg_option_int(yobj, key, default=0):
95 def get_linux_distro():
96 distro_name = ''
97 distro_version = ''
98+ flavor = ''
99 if os.path.exists('/etc/os-release'):
100- os_release = load_file('/etc/os-release')
101- for line in os_release.splitlines():
102- if line.strip().startswith('ID='):
103- distro_name = line.split('=')[-1]
104- distro_name = distro_name.replace('"', '')
105- if line.strip().startswith('VERSION_ID='):
106- # Lets hope for the best that distros stay consistent ;)
107- distro_version = line.split('=')[-1]
108- distro_version = distro_version.replace('"', '')
109+ os_release = load_shell_content(load_file('/etc/os-release'))
110+ distro_name = os_release.get('ID', '')
111+ distro_version = os_release.get('VERSION_ID', '')
112+ if 'sles' in distro_name or 'suse' in distro_name:
113+ # RELEASE_BLOCKER: We will drop this sles ivergent behavior in
114+ # before 18.4 so that get_linux_distro returns a named tuple
115+ # which will include both version codename and architecture
116+ # on all distributions.
117+ flavor = platform.machine()
118+ else:
119+ flavor = os_release.get('VERSION_CODENAME', '')
120+ if not flavor:
121+ match = re.match(r'[^ ]+ \((?P<codename>[^)]+)\)',
122+ os_release.get('VERSION'))
123+ if match:
124+ flavor = match.groupdict()['codename']
125 else:
126 dist = ('', '', '')
127 try:
128@@ -606,7 +614,7 @@ def get_linux_distro():
129 'expansion may have unexpected results')
130 return dist
131
132- return (distro_name, distro_version, platform.machine())
133+ return (distro_name, distro_version, flavor)
134
135
136 def system_info():
137diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py
138index af9d3e1..26b2b93 100644
139--- a/tests/unittests/test_datasource/test_azure_helper.py
140+++ b/tests/unittests/test_datasource/test_azure_helper.py
141@@ -85,7 +85,9 @@ class TestFindEndpoint(CiTestCase):
142 self.dhcp_options.return_value = {"eth0": {"unknown_245": "5:4:3:2"}}
143 self.assertEqual('5.4.3.2', wa_shim.find_endpoint(None))
144
145- def test_latest_lease_used(self):
146+ @mock.patch('cloudinit.sources.helpers.azure.util.is_FreeBSD')
147+ def test_latest_lease_used(self, m_is_freebsd):
148+ m_is_freebsd.return_value = False # To avoid hitting load_file
149 encoded_addresses = ['5:4:3:2', '4:3:2:1']
150 file_content = '\n'.join([self._build_lease_content(encoded_address)
151 for encoded_address in encoded_addresses])

Subscribers

People subscribed via source and target branches