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

facts: solaris: introduce distribution_major version detection for Solaris #43978

Open
wants to merge 17 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@mator
Contributor

mator commented Aug 10, 2018

facts: solaris: introduce distribution_major version detection for Solaris

Currently, there's no distribution_major in facts module on Solaris OS.
Use "uname -r" output to report major version.

Before the patch we get this on Solaris 11.3 :

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {}, "changed": false}

and after this patch, output is the following:

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {"ansible_distribution_major_version": "11"}, "changed": false}

Tested with Solaris 11.3 and Solaris 10 (both are x86_64 VMs)

Includes patch for test/units.

Fixes #18197

SUMMARY
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/facts/system/distribution.py

ANSIBLE VERSION
ansible 2.7.0.dev0 (devel e583484d59) last updated 2018/08/10 22:46:38 (GMT +300)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/mator/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/mator/ansible/lib/ansible
  executable location = /home/mator/ansible/bin/ansible
  python version = 2.7.15 (default, Jul 28 2018, 11:29:29) [GCC 8.1.0]
ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 11, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/facts/system/distribution.py:548:0: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/distribution.py:548:87: W291 trailing whitespace
lib/ansible/module_utils/facts/system/distribution.py:558:13: E265 block comment should start with '# '

click here for bot help

@mator

This comment has been minimized.

Contributor

mator commented Aug 11, 2018

do i need to resubmit with a cleaner commit history ? Thanks

@samdoran

This comment has been minimized.

Member

samdoran commented Aug 13, 2018

No, the commits will be squashed when merged.

lib/ansible/module_utils/facts/system/distribution.py Outdated
# For solaris 10 it will return 5.10, for solaris 11 it will be 5.11
# but I can't pass ansible test unit test_distribution_version.py with it,
# since I've no idea how to mock `uname -r` on CI (github/shippable) run.
# rc, uname_r, err = self.module.run_command('uname -r')

This comment has been minimized.

@samdoran

This comment has been minimized.

@mattclay

mattclay Sep 10, 2018

Member

Was there something in particular you wanted me to look at?

This comment has been minimized.

@mattclay

mattclay Sep 10, 2018

Member

Ah, let me see if I can find an example of mocking run_command.

@samdoran samdoran removed the needs_triage label Aug 13, 2018

@mator

This comment has been minimized.

Contributor

mator commented Aug 20, 2018

ping

@ansibot ansibot added the stale_ci label Aug 20, 2018

@mator

This comment has been minimized.

Contributor

mator commented Sep 10, 2018

@samdoran any chance we can proceed without reply from @mattclay ?

Thanks!

@samdoran

This comment has been minimized.

Member

samdoran commented Sep 10, 2018

@mator See @mattclay's suggestion on how to mock the uname output in the tests.

@mator

This comment has been minimized.

Contributor

mator commented Sep 13, 2018

thanks. going to try it and post a feedback later

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 14, 2018

@mator this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot removed the needs_rebase label Sep 15, 2018

@mator

This comment has been minimized.

Contributor

mator commented Sep 15, 2018

Should be a final version. @samdoran can you please review. Thanks!

tested with:

$ ansible --version
ansible 2.8.0.dev0 (devel 185f04db7d) last updated 2018/09/16 00:37:40 (GMT +300)
  config file = /home/mator/ans/ansible.cfg
  configured module search path = [u'/home/mator/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/mator/ansible/lib/ansible
  executable location = /home/mator/ansible/bin/ansible
  python version = 2.7.15+ (default, Aug 31 2018, 11:56:52) [GCC 8.2.0]

@ansibot ansibot removed the needs_revision label Sep 15, 2018

@samdoran

Please create a changelog fragment. See fragments for examples.

@@ -32,6 +32,13 @@ def get_uname_version(module):
return None
def get_uname_release(module):

This comment has been minimized.

@samdoran

samdoran Sep 17, 2018

Member

Rather than creating a new function just to change the uname flag, let's change the method signature for get_uname_version() to accept flags.

def get_uname_version(module, flags=('-v',)):
    if isinstance(flags, str):
        flags = flags.split()
    command = ['uname']
    command.extend(flags)
    rc, out, err = module.run_command(command)
    if rc == 0:
        return out
    return None

This comment has been minimized.

@mator

mator Oct 25, 2018

Contributor

@samdoran , using a single function for both uname vars breaks test unit, since i've no idea how to mock uname_v and uname_r with just one function with parameters, from test/units/module_utils/test_distribution_version.py :

mocker.patch('ansible.module_utils.facts.system.distribution.get_uname_version', mock_get_uname_version)
mocker.patch('ansible.module_utils.facts.system.distribution.get_uname_release', mock_get_uname_release)

This comment has been minimized.

@mator

mator Oct 25, 2018

Contributor

@mattclay can you please suggest how do i mock 2 different vars with one function call (with parameters) ? Thanks!

This comment has been minimized.

@samdoran

samdoran Nov 12, 2018

Member

Here is an example of how you could do it:

diff --git a/test/units/module_utils/test_distribution_version.py b/test/units/module_utils/test_distribution_version.py
index 99dbeadc42..99295a8a8f 100644
--- a/test/units/module_utils/test_distribution_version.py
+++ b/test/units/module_utils/test_distribution_version.py
@@ -1022,11 +1022,13 @@ def test_distribution_version(am, mocker, testcase):
             data = data.strip()
         return data
 
-    def mock_get_uname_version(am):
-        return testcase.get('uname_v', None)
-
-    def mock_get_uname_release(am):
-        return testcase.get('uname_r', None)
+    def mock_get_uname(am, flags):
+        if '-v' in flags:
+            return testcase.get('uname_v', None)
+        elif '-r' in flags:
+            return testcase.get('uname_r', None)
+        else:
+            return None
 
     def mock_file_exists(fname, allow_empty=False):
         if fname not in testcase['input']:
@@ -1046,8 +1048,7 @@ def test_distribution_version(am, mocker, testcase):
         return testcase.get('platform.version', '')
 
     mocker.patch('ansible.module_utils.facts.system.distribution.get_file_content', mock_get_file_content)
-    mocker.patch('ansible.module_utils.facts.system.distribution.get_uname_version', mock_get_uname_version)
-    mocker.patch('ansible.module_utils.facts.system.distribution.get_uname_release', mock_get_uname_release)
+    mocker.patch('ansible.module_utils.facts.system.distribution.get_uname', mock_get_uname)
     mocker.patch('ansible.module_utils.facts.system.distribution._file_exists', mock_file_exists)
     mocker.patch('platform.dist', lambda: testcase['platform.dist'])
     mocker.patch('platform.system', mock_platform_system)
Show resolved Hide resolved lib/ansible/module_utils/facts/system/distribution.py Outdated

mator added some commits Aug 10, 2018

facts: solaris: introduce distribution_major version detection for So…
…laris

Currently, there's no distribution_major in facts module on Solaris OS.
Use "uname -r" output to report major version.

Before the patch we get this on Solaris 11.3 :

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {}, "changed": false}

and after this patch, output is the following:

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {"ansible_distribution_major_version": "11"}, "changed": false}

Tested with Solaris 11.3 and Solaris 10 (both are x86_64 VMs)

Includes patch for test/units.

Fixes #18197
facts: solaris: introduce distribution_major version detection for So…
…laris

Currently, there's no distribution_major in facts module on Solaris OS.
Use "uname -r" output to report major version.

Before the patch we get this on Solaris 11.3 :

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {}, "changed": false}

and after this patch, output is the following:

$ ansible -o solaris11 -m setup -a filter=ansible_distribution_major_version
solaris11 | SUCCESS => {"ansible_facts": {"ansible_distribution_major_version": "11"}, "changed": false}

Tested with Solaris 11.3 and Solaris 10 (both are x86_64 VMs)

Includes patch for test/units.

Fixes #18197

@mator mator force-pushed the mator:devel branch from 185f04d to 04d3fdd Oct 25, 2018

@ansibot ansibot removed the stale_ci label Oct 25, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 25, 2018

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/distribution.py:560:43: bad-whitespace Exactly one space required after comma             uname_r = get_uname(self.module,flags=['-r'])                                            ^
lib/ansible/module_utils/facts/system/distribution.py:571:39: bad-whitespace Exactly one space required after comma         uname_v = get_uname(self.module,flags=['-v'])                                        ^

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/module_utils/facts/system/distribution.py:560:44: E231 missing whitespace after ','
lib/ansible/module_utils/facts/system/distribution.py:571:40: E231 missing whitespace after ','

click here for bot help

@ansibot ansibot added the stale_ci label Nov 3, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment