code cleanup for --diff and --check modes (#33665)

* code cleanup for `--diff` and `--check` modes

* fixes UT to remove exec_command

lib/ansible/module_utils/network/iosxr/iosxr.py

@@ -336,7 +336,7 @@ def commit_config(module, comment=None, confirmed=False, confirm_timeout=None, p
     return reply
 
 
-def get_config(module, source='running', config_filter=None):
+def get_config(module, config_filter=None, source='running'):
     global _DEVICE_CONFIGS
 
     conn = get_connection(module)
@@ -358,20 +358,30 @@ def get_config(module, source='running', config_filter=None):
         return cfg
 
 
-def load_config(module, command_filter, warnings, replace=False, admin=False, commit=False, comment=None):
+def load_config(module, command_filter, commit=False, replace=False,
+                comment=None, admin=False, running=None, nc_get_filter=None):
+
     conn = get_connection(module)
 
+    diff = None
     if is_netconf(module):
         # FIXME: check for platform behaviour and restore this
-        # ret = conn.lock(target = 'candidate')
+        # conn.lock(target = 'candidate')
-        # ret = conn.discard_changes()
+        # conn.discard_changes()
-        try:
-            ret = conn.edit_config(command_filter)
-        finally:
-            # ret = conn.unlock(target = 'candidate')
-            pass
 
-        return ret
+        try:
+            conn.edit_config(command_filter)
+
+            candidate = get_config(module, source='candidate', config_filter=nc_get_filter)
+            diff = get_config_diff(module, running, candidate)
+
+            if commit and diff:
+                commit_config(module)
+            else:
+                discard_config(module)
+        finally:
+            # conn.unlock(target = 'candidate')
+            pass
 
     elif is_cliconf(module):
         # to keep the pre-cliconf behaviour, make a copy, avoid adding commands to input list
@@ -380,17 +390,17 @@ def load_config(module, command_filter, warnings, replace=False, admin=False, co
         if admin:
             cmd_filter.insert(0, 'admin')
         conn.edit_config(cmd_filter)
-        diff = get_config_diff(module)
+
         if module._diff:
-            if diff:
+            diff = get_config_diff(module)
-                module._result['diff'] = to_text(diff, errors='surrogate_or_strict')
+
         if commit:
             commit_config(module, comment=comment)
             conn.edit_config('end')
         else:
             conn.discard_changes()
 
-        return diff
+    return diff
 
 
 def run_command(module, commands):

lib/ansible/modules/network/iosxr/iosxr_banner.py

@@ -83,10 +83,9 @@ import collections
 
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.network.iosxr.iosxr import get_config, load_config
-from ansible.module_utils.network.iosxr.iosxr import get_config_diff, commit_config
 from ansible.module_utils.network.iosxr.iosxr import iosxr_argument_spec, discard_config
 from ansible.module_utils.network.iosxr.iosxr import build_xml, is_cliconf, is_netconf
-from ansible.module_utils.network.iosxr.iosxr import etree_find, etree_findall
+from ansible.module_utils.network.iosxr.iosxr import etree_find
 
 
 class ConfigBase(object):
@@ -125,8 +124,10 @@ class CliConfiguration(ConfigBase):
                 commands.append(banner_cmd)
         self._result['commands'] = commands
         if commands:
-            if not self._module.check_mode:
+            commit = not self._module.check_mode
-                load_config(self._module, commands, self._result['warnings'], commit=True)
+            diff = load_config(self._module, commands, commit=commit)
+            if diff:
+                self._result['diff'] = dict(prepared=diff)
             self._result['changed'] = True
 
     def map_config_to_obj(self):
@@ -184,19 +185,15 @@ class NCConfiguration(ConfigBase):
             _edit_filter = build_xml('banners', xmap=self._banners_meta, params=self._module.params, opcode=opcode)
 
             if _edit_filter is not None:
-                if not self._module.check_mode:
+                commit = not self._module.check_mode
-                    load_config(self._module, _edit_filter, self._result['warnings'])
+                diff = load_config(self._module, _edit_filter, commit=commit, running=running, nc_get_filter=_get_filter)
-                    candidate = get_config(self._module, source='candidate', config_filter=_get_filter)
 
-                    diff = get_config_diff(self._module, running, candidate)
+                if diff:
-                    if diff:
+                    self._result['commands'] = _edit_filter
-                        commit_config(self._module)
+                    if self._module._diff:
-                        self._result['changed'] = True
+                        self._result['diff'] = dict(prepared=diff)
-                        self._result['commands'] = _edit_filter
+
-                        if self._module._diff:
+                    self._result['changed'] = True
-                            self._result['diff'] = {'prepared': diff}
-                    else:
-                        discard_config(self._module)
 
     def run(self):
         self.map_params_to_obj()
@@ -223,7 +220,7 @@ def main():
                            supports_check_mode=True)
 
     if is_cliconf(module):
-        module.deprecate("cli support for 'iosxr_banner' is deprecated. Use transport netconf instead', version='4 releases from v2.5")
+        module.deprecate(msg="cli support for 'iosxr_banner' is deprecated. Use transport netconf instead", version="4 releases from v2.5")
         config_object = CliConfiguration(module)
     elif is_netconf(module):
         config_object = NCConfiguration(module)

lib/ansible/modules/network/iosxr/iosxr_config.py

@@ -246,11 +246,12 @@ def run(module, result):
 
             result['commands'] = commands
 
-        diff = load_config(module, commands, result['warnings'],
+        commit = not check_mode
-                           not check_mode, replace_config, comment, admin)
+        diff = load_config(module, commands, commit=commit, replace=replace_config,
+                           comment=comment, admin=admin)
         if diff:
             result['diff'] = dict(prepared=diff)
-            result['changed'] = True
+        result['changed'] = True
 
 
 def main():

lib/ansible/modules/network/iosxr/iosxr_interface.py

@@ -400,8 +400,10 @@ def main():
     result['warnings'] = warnings
 
     if commands:
-        if not module.check_mode:
+        commit = not module.check_mode
-            load_config(module, commands, result['warnings'], commit=True)
+        diff = load_config(module, commands, commit=commit)
+        if diff:
+            result['diff'] = dict(prepared=diff)
         result['changed'] = True
 
     failed_conditions = check_declarative_intent_params(module, want, result)

lib/ansible/modules/network/iosxr/iosxr_logging.py

@@ -361,8 +361,10 @@ def main():
     result['warnings'] = warnings
 
     if commands:
-        if not module.check_mode:
+        commit = not module.check_mode
-            load_config(module, commands, result['warnings'], commit=True)
+        diff = load_config(module, commands, commit=commit)
+        if diff:
+            result['diff'] = dict(prepared=diff)
         result['changed'] = True
 
     module.exit_json(**result)

lib/ansible/modules/network/iosxr/iosxr_netconf.py

@@ -74,7 +74,6 @@ commands:
 import re
 
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.connection import exec_command
 from ansible.module_utils.network.iosxr.iosxr import iosxr_argument_spec
 from ansible.module_utils.network.iosxr.iosxr import get_config, load_config
 from ansible.module_utils.six import iteritems
@@ -82,7 +81,7 @@ from ansible.module_utils.six import iteritems
 USE_PERSISTENT_CONNECTION = True
 
 
-def map_obj_to_commands(updates, module):
+def map_obj_to_commands(updates):
     want, have = updates
     commands = list()
 
@@ -186,12 +185,14 @@ def main():
 
     want = map_params_to_obj(module)
     have = map_config_to_obj(module)
-    commands = map_obj_to_commands((want, have), module)
+    commands = map_obj_to_commands((want, have))
     result['commands'] = commands
 
     if commands:
-        if not module.check_mode:
+        commit = not module.check_mode
-            diff = load_config(module, commands, result['warnings'], commit=True)
+        diff = load_config(module, commands, commit=commit)
+        if diff:
+            result['diff'] = dict(prepared=diff)
         result['changed'] = True
 
     module.exit_json(**result)

lib/ansible/modules/network/iosxr/iosxr_system.py

@@ -242,8 +242,10 @@ def main():
     result['commands'] = commands
 
     if commands:
-        if not module.check_mode:
+        commit = not module.check_mode
-            load_config(module, commands, result['warnings'], commit=True)
+        diff = load_config(module, commands, commit=commit)
+        if diff:
+            result['diff'] = dict(prepared=diff)
         result['changed'] = True
 
     module.exit_json(**result)

lib/ansible/modules/network/iosxr/iosxr_user.py

@@ -475,8 +475,10 @@ def main():
         module.fail_json(msg='cannot delete the `admin` account')
 
     if commands:
-        if not module.check_mode:
+        commit = not module.check_mode
-            load_config(module, commands, result['warnings'], commit=True)
+        diff = load_config(module, commands, commit=commit)
+        if diff:
+            result['diff'] = dict(prepared=diff)
         result['changed'] = True
 
     if module.params['state'] == 'present' and (module.params['public_key_contents'] or module.params['public_key']):

test/units/modules/network/iosxr/test_iosxr_netconf.py

@@ -32,9 +32,6 @@ class TestIosxrNetconfModule(TestIosxrModule):
     def setUp(self):
         super(TestIosxrNetconfModule, self).setUp()
 
-        self.mock_exec_command = patch('ansible.modules.network.iosxr.iosxr_netconf.exec_command')
-        self.exec_command = self.mock_exec_command.start()
-
         self.mock_get_config = patch('ansible.modules.network.iosxr.iosxr_netconf.get_config')
         self.get_config = self.mock_get_config.start()
 
@@ -43,7 +40,6 @@ class TestIosxrNetconfModule(TestIosxrModule):
 
     def tearDown(self):
         super(TestIosxrNetconfModule, self).tearDown()
-        self.mock_exec_command.stop()
         self.mock_get_config.stop()
         self.mock_load_config.stop()
 
@@ -54,14 +50,14 @@ class TestIosxrNetconfModule(TestIosxrModule):
         !
         ssh server netconf vrf default
         '''
-        self.exec_command.return_value = 0, '', None
+        self.load_config.return_value = 'dummy diff'
         set_module_args(dict(netconf_port=830, netconf_vrf='default', state='absent'))
         result = self.execute_module(changed=True)
         self.assertEqual(result['commands'], ['no netconf-yang agent ssh', 'no ssh server netconf port 830', 'no ssh server netconf vrf default'])
 
     def test_iosxr_enable_netconf_service(self):
         self.get_config.return_value = ''
-        self.exec_command.return_value = 0, '', None
+        self.load_config.return_value = 'dummy diff'
         set_module_args(dict(netconf_port=830, netconf_vrf='default', state='present'))
         result = self.execute_module(changed=True)
         self.assertEqual(result['commands'], ['netconf-yang agent ssh', 'ssh server netconf port 830', 'ssh server netconf vrf default'])
@@ -73,7 +69,7 @@ class TestIosxrNetconfModule(TestIosxrModule):
         !
         ssh server netconf vrf default
         '''
-        self.exec_command.return_value = 0, '', None
+        self.load_config.return_value = 'dummy diff'
         set_module_args(dict(netconf_port=9000, state='present'))
         result = self.execute_module(changed=True)
         self.assertEqual(result['commands'], ['ssh server netconf port 9000'])
@@ -85,7 +81,7 @@ class TestIosxrNetconfModule(TestIosxrModule):
         !
         ssh server netconf vrf default
         '''
-        self.exec_command.return_value = 0, '', None
+        self.load_config.return_value = 'dummy diff'
         set_module_args(dict(netconf_vrf='new_default', state='present'))
         result = self.execute_module(changed=True)
         self.assertEqual(result['commands'], ['ssh server netconf vrf new_default'])