Merge pull request #32254 from twangboy/fix_31334

Fix Display Name with spaces in win_servermanager

salt/modules/win_servermanager.py

@@ -4,6 +4,7 @@ Manage Windows features via the ServerManager powershell module
 '''
 from __future__ import absolute_import
 import logging
+import json
 
 # Import python libs
 try:
@@ -24,13 +25,13 @@ def __virtual__():
     Load only on windows with servermanager module
     '''
     if not salt.utils.is_windows():
-        return (False, 'Failed to load win_servermanager module:\n'
-                       'Only available on Windows systems.')
+        return False, 'Failed to load win_servermanager module:\n' \
+                      'Only available on Windows systems.'
 
     if not _check_server_manager():
-        return (False, 'Failed to load win_servermanager module:\n'
-                       'ServerManager module not available.\n'
-                       'May need to install Remote Server Administration Tools.')
+        return False, 'Failed to load win_servermanager module:\n' \
+                      'ServerManager module not available.\n' \
+                      'May need to install Remote Server Administration Tools.'
 
     return __virtualname__
 
@@ -46,31 +47,19 @@ def _check_server_manager():
                                        python_shell=True)
 
 
-def _pshell(func):
+def _pshell_json(cmd, cwd=None):
     '''
-    Execute a powershell command and return the STDOUT
+    Execute the desired powershell command and ensure that it returns data
+    in json format and load that into python
     '''
-    return __salt__['cmd.run']('{0}'.format(func),
-                               shell='powershell',
-                               python_shell=True)
-
-
-def _parse_powershell_list(lst):
-    '''
-    Parse command output when piped to format-list
-    Need to look at splitting with ':' so you can get the full value
-    Need to check for error codes and return false if it's trying to parse
-    '''
-    ret = {}
-    for line in lst.splitlines():
-        if line:
-            splt = line.split()
-            # Ensure it's not a malformed line, e.g.:
-            #   FeatureResult : {foo, bar,
-            #                    baz}
-            if len(splt) > 2:
-                ret[splt[0]] = splt[2]
-    ret['message'] = lst
+    if 'convertto-json' not in cmd.lower():
+        cmd = ' '.join([cmd, '| ConvertTo-Json'])
+    log.debug('PowerShell: {0}'.format(cmd))
+    ret = __salt__['cmd.shell'](cmd, shell='powershell', cwd=cwd)
+    try:
+        ret = json.loads(ret, strict=False)
+    except ValueError:
+        log.debug('Json not returned')
     return ret
 
 
@@ -87,8 +76,9 @@ def list_available():
 
         salt '*' win_servermanager.list_available
     '''
-    return _pshell('Get-WindowsFeature -erroraction silentlycontinue '
-                   '-warningaction silentlycontinue')
+    cmd = 'Get-WindowsFeature -erroraction silentlycontinue ' \
+          '-warningaction silentlycontinue'
+    return __salt__['cmd.shell'](cmd, shell='powershell')
 
 
 def list_installed():
@@ -105,22 +95,16 @@ def list_installed():
 
         salt '*' win_servermanager.list_installed
     '''
+    cmd = 'Get-WindowsFeature -erroraction silentlycontinue ' \
+          '-warningaction silentlycontinue | ' \
+          'Select DisplayName,Name,Installed'
+    features = _pshell_json(cmd)
+
     ret = {}
-    names = _pshell('Get-WindowsFeature -erroraction silentlycontinue '
-                    '-warningaction silentlycontinue | Select DisplayName,Name')
-    for line in names.splitlines()[2:]:
-        splt = line.split()
-        name = splt.pop(-1)
-        display_name = ' '.join(splt)
-        ret[name] = display_name
-    state = _pshell('Get-WindowsFeature -erroraction silentlycontinue '
-                    '-warningaction silentlycontinue | Select Installed,Name')
-    for line in state.splitlines()[2:]:
-        splt = line.split()
-        if splt[0] == 'False' and splt[1] in ret:
-            del ret[splt[1]]
-        if '----' in splt[0]:
-            del ret[splt[1]]
+    for entry in features:
+        if entry['Installed']:
+            ret[entry['Name']] = entry['DisplayName']
+
     return ret
 
 
@@ -153,11 +137,18 @@ def install(feature, recurse=False):
     sub = ''
     if recurse:
         sub = '-IncludeAllSubFeature'
-    out = _pshell('Add-WindowsFeature -Name {0} {1} '
-                  '-erroraction silentlycontinue '
-                  '-warningaction silentlycontinue '
-                  '| format-list'.format(_cmd_quote(feature), sub))
-    return _parse_powershell_list(out)
+
+    cmd = 'Add-WindowsFeature -Name {0} {1} ' \
+          '-ErrorAction SilentlyContinue ' \
+          '-WarningAction SilentlyContinue'.format(_cmd_quote(feature), sub)
+    out = _pshell_json(cmd)
+
+    ret = {'ExitCode': out['ExitCode'],
+           'DisplayName': out['FeatureResult'][0]['DisplayName'],
+           'RestartNeeded': out['FeatureResult'][0]['RestartNeeded'],
+           'Success': out['Success']}
+
+    return ret
 
 
 def remove(feature):
@@ -182,8 +173,14 @@ def remove(feature):
 
         salt -t 600 '*' win_servermanager.remove Telnet-Client
     '''
-    out = _pshell('Remove-WindowsFeature -Name {0} '
-                  '-erroraction silentlycontinue '
-                  '-warningaction silentlycontinue '
-                  '| format-list'.format(_cmd_quote(feature)))
-    return _parse_powershell_list(out)
+    cmd = 'Remove-WindowsFeature -Name {0} ' \
+          '-ErrorAction SilentlyContinue ' \
+          '-WarningAction SilentlyContinue'.format(_cmd_quote(feature))
+    out = _pshell_json(cmd)
+
+    ret = {'ExitCode': out['ExitCode'],
+           'DisplayName': out['FeatureResult'][0]['DisplayName'],
+           'RestartNeeded': out['FeatureResult'][0]['RestartNeeded'],
+           'Success': out['Success']}
+
+    return ret

salt/states/win_servermanager.py

@@ -65,14 +65,14 @@ def installed(name, recurse=False, force=False):
     ret['changes'] = {'feature': __salt__['win_servermanager.install'](name, recurse)}
 
     if 'Success' in ret['changes']['feature']:
-        ret['result'] = ret['changes']['feature']['Success'] == 'True'
+        ret['result'] = ret['changes']['feature']['Success']
         if not ret['result']:
             ret['comment'] = 'Failed to install {0}: {1}'.format(name, ret['changes']['feature']['ExitCode'])
         else:
             ret['comment'] = 'Installed {0}'.format(name)
     else:
         ret['result'] = False
-        ret['comment'] = 'Failed to install {0}.\nError Message:\n{1}'.format(name, ret['changes']['feature']['message'])
+        ret['comment'] = 'Failed to install {0}.\nError Message:\n{1}'.format(name, ret['changes']['feature'])
         ret['changes'] = {}
 
     return ret
@@ -119,7 +119,7 @@ def removed(name):
 
     # Remove the features
     ret['changes'] = {'feature': __salt__['win_servermanager.remove'](name)}
-    ret['result'] = ret['changes']['feature']['Success'] == 'True'
+    ret['result'] = ret['changes']['feature']['Success']
     if not ret['result']:
         ret['comment'] = 'Failed to uninstall the feature {0}'.format(ret['changes']['feature']['ExitCode'])
 

tests/unit/modules/win_servermanager_test.py

@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 '''
-    :codeauthor: :email:`Jayesh Kariya <jayeshk@saltstack.com>`
+:codeauthor: :email:`Jayesh Kariya <jayeshk@saltstack.com>`
 '''
 
 # Import Python Libs
@@ -8,13 +8,7 @@ from __future__ import absolute_import
 
 # Import Salt Testing Libs
 from salttesting import TestCase, skipIf
-from salttesting.mock import (
-    MagicMock,
-    patch,
-    NO_MOCK,
-    NO_MOCK_REASON
-)
-
+from salttesting.mock import MagicMock, patch, NO_MOCK, NO_MOCK_REASON
 from salttesting.helpers import ensure_in_syspath
 
 ensure_in_syspath('../../')
@@ -31,52 +25,61 @@ class WinServermanagerTestCase(TestCase):
     '''
     Test cases for salt.modules.win_servermanager
     '''
-    @staticmethod
-    def _m_run():
-        '''
-        Mock return value for cmd.run
-        '''
-        return MagicMock(return_value='')
-
-    # 'list_available' function tests: 1
-
     def test_list_available(self):
         '''
-        Test if it list available features to install.
+        Test win_servermanager.list_available
         '''
-        with patch.dict(win_servermanager.__salt__, {'cmd.run': self._m_run()}):
+        mock = MagicMock(return_value='')
+        with patch.dict(win_servermanager.__salt__, {'cmd.shell': mock}):
             self.assertEqual(win_servermanager.list_available(), '')
 
-    # 'list_installed' function tests: 1
-
     def test_list_installed(self):
         '''
-        Test if it list installed features.
+        Test win_servermanager.list_installed
         '''
-        with patch.dict(win_servermanager.__salt__, {'cmd.run': self._m_run()}):
-            self.assertDictEqual(win_servermanager.list_installed(), {})
-
-    # 'install' function tests: 1
+        mock = MagicMock(return_value=[{'Installed': True,
+                                        'Name': 'Spongebob',
+                                        'DisplayName': 'Square Pants'},
+                                       {'Installed': False,
+                                        'Name': 'Patrick',
+                                        'DisplayName': 'Plankton'}])
+        with patch.object(win_servermanager, '_pshell_json', mock):
+            expected = {'Spongebob': 'Square Pants'}
+            self.assertDictEqual(win_servermanager.list_installed(), expected)
 
     def test_install(self):
         '''
-        Test if it install a feature.
+        Test win_servermanager.install
         '''
-        with patch.dict(win_servermanager.__salt__, {'cmd.run': self._m_run()}):
-            self.assertDictEqual(win_servermanager.install('Telnet-Client'), {
-                'message': ''
-            })
-
-    # 'remove' function tests: 1
+        mock = MagicMock(return_value={'ExitCode': 0,
+                                       'Success': True,
+                                       'FeatureResult':
+                                           [{'DisplayName': 'Spongebob',
+                                             'RestartNeeded': False}]})
+        with patch.object(win_servermanager, '_pshell_json', mock):
+            expected = {'ExitCode': 0,
+                        'DisplayName': 'Spongebob',
+                        'RestartNeeded': False,
+                        'Success': True}
+            self.assertDictEqual(
+                win_servermanager.install('Telnet-Client'), expected)
 
     def test_remove(self):
         '''
-        Test if it remove an installed feature.
+        Test win_servermanager.remove
         '''
-        with patch.dict(win_servermanager.__salt__, {'cmd.run': self._m_run()}):
-            self.assertDictEqual(win_servermanager.remove('Telnet-Client'), {
-                'message': ''
-            })
+        mock = MagicMock(return_value={'ExitCode': 0,
+                                       'Success': True,
+                                       'FeatureResult':
+                                           [{'DisplayName': 'Spongebob',
+                                             'RestartNeeded': False}]})
+        with patch.object(win_servermanager, '_pshell_json', mock):
+            expected = {'ExitCode': 0,
+                        'DisplayName': 'Spongebob',
+                        'RestartNeeded': False,
+                        'Success': True}
+            self.assertDictEqual(
+                win_servermanager.remove('Telnet-Client'), expected)
 
 
 if __name__ == '__main__':

tests/unit/states/win_servermanager_test.py

@@ -40,7 +40,7 @@ class WinServermanagerTestCase(TestCase):
                'result': True,
                'comment': ''}
         mock = MagicMock(side_effect=['salt', 'stack', 'stack'])
-        mock1 = MagicMock(return_value={'Success': 'True'})
+        mock1 = MagicMock(return_value={'Success': True})
         with patch.dict(win_servermanager.__salt__,
                         {"win_servermanager.list_installed": mock,
                          "win_servermanager.install": mock1}):
@@ -55,7 +55,7 @@ class WinServermanagerTestCase(TestCase):
                 self.assertDictEqual(win_servermanager.installed('salt'), ret)
 
                 with patch.dict(win_servermanager.__opts__, {"test": False}):
-                    ret.update({'changes': {'feature': {'Success': 'True'}},
+                    ret.update({'changes': {'feature': {'Success': True}},
                                 'result': True, 'comment': 'Installed salt'})
                     self.assertDictEqual(win_servermanager.installed('salt'),
                                          ret)
@@ -69,7 +69,7 @@ class WinServermanagerTestCase(TestCase):
                'result': True,
                'comment': ''}
         mock = MagicMock(side_effect=['stack', 'salt', 'salt'])
-        mock1 = MagicMock(return_value={'Success': 'True'})
+        mock1 = MagicMock(return_value={'Success': True})
         with patch.dict(win_servermanager.__salt__,
                         {"win_servermanager.list_installed": mock,
                          "win_servermanager.remove": mock1}):
@@ -83,7 +83,7 @@ class WinServermanagerTestCase(TestCase):
                 self.assertDictEqual(win_servermanager.removed('salt'), ret)
 
                 with patch.dict(win_servermanager.__opts__, {"test": False}):
-                    ret.update({'changes': {'feature': {'Success': 'True'}},
+                    ret.update({'changes': {'feature': {'Success': True}},
                                 'result': True})
                     self.assertDictEqual(win_servermanager.removed('salt'),
                                          ret)