From 012a96dabda4da308ca4f35fb14bab3397e6154f Mon Sep 17 00:00:00 2001 From: Kedar Kekan <4506537+kedarX@users.noreply.github.com> Date: Thu, 7 Dec 2017 20:14:57 +0530 Subject: [PATCH] code cleanup for `--diff` and `--check` modes (#33665) * code cleanup for `--diff` and `--check` modes * fixes UT to remove exec_command --- .../module_utils/network/iosxr/iosxr.py | 38 ++++++++++++------- .../modules/network/iosxr/iosxr_banner.py | 31 +++++++-------- .../modules/network/iosxr/iosxr_config.py | 7 ++-- .../modules/network/iosxr/iosxr_interface.py | 6 ++- .../modules/network/iosxr/iosxr_logging.py | 6 ++- .../modules/network/iosxr/iosxr_netconf.py | 11 +++--- .../modules/network/iosxr/iosxr_system.py | 6 ++- .../modules/network/iosxr/iosxr_user.py | 6 ++- .../network/iosxr/test_iosxr_netconf.py | 12 ++---- 9 files changed, 68 insertions(+), 55 deletions(-) diff --git a/lib/ansible/module_utils/network/iosxr/iosxr.py b/lib/ansible/module_utils/network/iosxr/iosxr.py index 6fd2914704..805acf8512 100644 --- a/lib/ansible/module_utils/network/iosxr/iosxr.py +++ b/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') - # ret = conn.discard_changes() - try: - ret = conn.edit_config(command_filter) - finally: - # ret = conn.unlock(target = 'candidate') - pass + # conn.lock(target = 'candidate') + # conn.discard_changes() - 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: - module._result['diff'] = to_text(diff, errors='surrogate_or_strict') + diff = get_config_diff(module) + if commit: commit_config(module, comment=comment) conn.edit_config('end') else: conn.discard_changes() - return diff + return diff def run_command(module, commands): diff --git a/lib/ansible/modules/network/iosxr/iosxr_banner.py b/lib/ansible/modules/network/iosxr/iosxr_banner.py index 264bdb7fd5..108e2b66ef 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_banner.py +++ b/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: - load_config(self._module, commands, self._result['warnings'], commit=True) + commit = not self._module.check_mode + 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: - load_config(self._module, _edit_filter, self._result['warnings']) - candidate = get_config(self._module, source='candidate', config_filter=_get_filter) + commit = not self._module.check_mode + diff = load_config(self._module, _edit_filter, commit=commit, running=running, nc_get_filter=_get_filter) - diff = get_config_diff(self._module, running, candidate) - if diff: - commit_config(self._module) - self._result['changed'] = True - self._result['commands'] = _edit_filter - if self._module._diff: - self._result['diff'] = {'prepared': diff} - else: - discard_config(self._module) + if diff: + self._result['commands'] = _edit_filter + if self._module._diff: + self._result['diff'] = dict(prepared=diff) + + self._result['changed'] = True 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) diff --git a/lib/ansible/modules/network/iosxr/iosxr_config.py b/lib/ansible/modules/network/iosxr/iosxr_config.py index d886a7cc7b..9da1aca6a6 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_config.py +++ b/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'], - not check_mode, replace_config, comment, admin) + commit = not check_mode + 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(): diff --git a/lib/ansible/modules/network/iosxr/iosxr_interface.py b/lib/ansible/modules/network/iosxr/iosxr_interface.py index 7404ff5ccf..14832fe3c2 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_interface.py +++ b/lib/ansible/modules/network/iosxr/iosxr_interface.py @@ -400,8 +400,10 @@ def main(): result['warnings'] = warnings if commands: - if not module.check_mode: - load_config(module, commands, result['warnings'], commit=True) + commit = not module.check_mode + 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) diff --git a/lib/ansible/modules/network/iosxr/iosxr_logging.py b/lib/ansible/modules/network/iosxr/iosxr_logging.py index 9a5c0a672f..a03b6fb53b 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_logging.py +++ b/lib/ansible/modules/network/iosxr/iosxr_logging.py @@ -361,8 +361,10 @@ def main(): result['warnings'] = warnings if commands: - if not module.check_mode: - load_config(module, commands, result['warnings'], commit=True) + commit = not module.check_mode + diff = load_config(module, commands, commit=commit) + if diff: + result['diff'] = dict(prepared=diff) result['changed'] = True module.exit_json(**result) diff --git a/lib/ansible/modules/network/iosxr/iosxr_netconf.py b/lib/ansible/modules/network/iosxr/iosxr_netconf.py index 4ba17299c6..7ab0984ac2 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_netconf.py +++ b/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: - diff = load_config(module, commands, result['warnings'], commit=True) + commit = not module.check_mode + diff = load_config(module, commands, commit=commit) + if diff: + result['diff'] = dict(prepared=diff) result['changed'] = True module.exit_json(**result) diff --git a/lib/ansible/modules/network/iosxr/iosxr_system.py b/lib/ansible/modules/network/iosxr/iosxr_system.py index 3043156e22..25245f230c 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_system.py +++ b/lib/ansible/modules/network/iosxr/iosxr_system.py @@ -242,8 +242,10 @@ def main(): result['commands'] = commands if commands: - if not module.check_mode: - load_config(module, commands, result['warnings'], commit=True) + commit = not module.check_mode + diff = load_config(module, commands, commit=commit) + if diff: + result['diff'] = dict(prepared=diff) result['changed'] = True module.exit_json(**result) diff --git a/lib/ansible/modules/network/iosxr/iosxr_user.py b/lib/ansible/modules/network/iosxr/iosxr_user.py index 0f514774db..b7a01918e9 100644 --- a/lib/ansible/modules/network/iosxr/iosxr_user.py +++ b/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: - load_config(module, commands, result['warnings'], commit=True) + commit = not module.check_mode + 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']): diff --git a/test/units/modules/network/iosxr/test_iosxr_netconf.py b/test/units/modules/network/iosxr/test_iosxr_netconf.py index 7cde357718..f7d7fa3120 100644 --- a/test/units/modules/network/iosxr/test_iosxr_netconf.py +++ b/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'])