diff --git a/changelogs/fragments/39-api-fail.yml b/changelogs/fragments/39-api-fail.yml new file mode 100644 index 0000000..f361485 --- /dev/null +++ b/changelogs/fragments/39-api-fail.yml @@ -0,0 +1,8 @@ +breaking_changes: +- >- + api - due to a programming error, the module never failed on errors. This has now been fixed. + If you are relying on the module not failing in case of idempotent commands + (resulting in errors like ``failure: already have such address``), you need to adjust your roles/playbooks. + We suggest to use ``failed_when`` to accept failure in specific circumstances, + for example ``failed_when: "'failure: already have ' in result.msg[0]"`` + (https://github.com/ansible-collections/community.routeros/pull/39). diff --git a/plugins/modules/api.py b/plugins/modules/api.py index cdfac7d..c25e6c8 100644 --- a/plugins/modules/api.py +++ b/plugins/modules/api.py @@ -266,6 +266,7 @@ import traceback LIB_IMP_ERR = None try: from librouteros import connect + from librouteros.exceptions import LibRouterosError from librouteros.query import Key HAS_LIB = True except Exception as e: @@ -373,7 +374,7 @@ class ROS_api_module: for i in self.api_path: self.result['message'].append(i) self.return_result(False, True) - except Exception as e: + except LibRouterosError as e: self.errors(e) def api_add(self): @@ -382,7 +383,7 @@ class ROS_api_module: self.result['message'].append("added: .id= %s" % self.api_path.add(**param)) self.return_result(True) - except Exception as e: + except LibRouterosError as e: self.errors(e) def api_remove(self): @@ -390,7 +391,7 @@ class ROS_api_module: self.api_path.remove(self.remove) self.result['message'].append("removed: .id= %s" % self.remove) self.return_result(True) - except Exception as e: + except LibRouterosError as e: self.errors(e) def api_update(self): @@ -401,7 +402,7 @@ class ROS_api_module: self.api_path.update(**param) self.result['message'].append("updated: %s" % param) self.return_result(True) - except Exception as e: + except LibRouterosError as e: self.errors(e) def api_query(self): @@ -440,7 +441,7 @@ class ROS_api_module: msg = msg + ' WHERE %s' % ' '.join(self.where) self.result['message'].append(msg) self.return_result(False) - except Exception as e: + except LibRouterosError as e: self.errors(e) def api_arbitrary(self): @@ -454,12 +455,12 @@ class ROS_api_module: for i in arbitrary_result: self.result['message'].append(i) self.return_result(False) - except Exception as e: + except LibRouterosError as e: self.errors(e) def return_result(self, ch_status=False, status=True): - if status == "False": - self.module.fail_json(msg=to_native(self.result['message'])) + if not status: + self.module.fail_json(msg=self.result['message']) else: self.module.exit_json(changed=ch_status, msg=self.result['message']) @@ -467,7 +468,7 @@ class ROS_api_module: def errors(self, e): if e.__class__.__name__ == 'TrapError': self.result['message'].append("%s" % e) - self.return_result(False, True) + self.return_result(False, False) self.result['message'].append("%s" % e) self.return_result(False, False) diff --git a/tests/unit/plugins/modules/test_api.py b/tests/unit/plugins/modules/test_api.py index 1befeff..cabb918 100644 --- a/tests/unit/plugins/modules/test_api.py +++ b/tests/unit/plugins/modules/test_api.py @@ -21,35 +21,23 @@ import json import pytest from ansible_collections.community.routeros.tests.unit.compat.mock import patch, MagicMock -from ansible_collections.community.routeros.tests.unit.plugins.modules.utils import set_module_args, basic, AnsibleExitJson, AnsibleFailJson, ModuleTestCase +from ansible_collections.community.routeros.tests.unit.plugins.modules.utils import set_module_args, AnsibleExitJson, AnsibleFailJson, ModuleTestCase from ansible_collections.community.routeros.plugins.modules import api -class AnsibleExitJson(Exception): - """Exception class to be raised by module.exit_json and caught by the test case""" - pass +class FakeLibRouterosError(Exception): + def __init__(self, message): + self.message = message + super(FakeLibRouterosError, self).__init__(self.message) -class AnsibleFailJson(Exception): - """Exception class to be raised by module.fail_json and caught by the test case""" - pass - - -def exit_json(*args, **kwargs): - """function to patch over exit_json; package return data into an exception""" - if 'changed' not in kwargs: - kwargs['changed'] = False - raise AnsibleExitJson(kwargs) - - -def fail_json(*args, **kwargs): - """function to patch over fail_json; package return data into an exception""" - kwargs['failed'] = True - raise AnsibleFailJson(kwargs) +class TrapError(FakeLibRouterosError): + def __init__(self, message="failure: already have interface with such name"): + super(TrapError, self).__init__(message) # fixtures -class fake_ros_api: +class fake_ros_api(object): def __init__(self, api, path): pass @@ -114,7 +102,7 @@ class fake_ros_api: return api_path -class Where: +class Where(object): def __init__(self): pass @@ -125,13 +113,7 @@ class Where: return ["*A1"] -class TrapError(Exception): - def __init__(self, message="failure: already have interface with such name"): - self.message = message - super().__init__(self.message) - - -class Key: +class Key(object): def __init__(self, name): self.name = name self.str_return() @@ -143,8 +125,10 @@ class Key: class TestRouterosApiModule(ModuleTestCase): def setUp(self): + super(TestRouterosApiModule, self).setUp() librouteros = pytest.importorskip("librouteros") self.module = api + self.module.LibRouterosError = FakeLibRouterosError self.module.connect = MagicMock(new=fake_ros_api) self.module.Key = MagicMock(new=Key) self.config_module_args = {"username": "admin", @@ -152,115 +136,155 @@ class TestRouterosApiModule(ModuleTestCase): "hostname": "127.0.0.1", "path": "interface bridge"} - self.mock_module_helper = patch.multiple(basic.AnsibleModule, - exit_json=exit_json, - fail_json=fail_json) - self.mock_module_helper.start() - self.addCleanup(self.mock_module_helper.stop) - def test_module_fail_when_required_args_missing(self): - with self.assertRaises(AnsibleFailJson): + with self.assertRaises(AnsibleFailJson) as exc: set_module_args({}) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['failed'], True) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.path) def test_api_path(self): - with self.assertRaises(AnsibleExitJson): - set_module_args(self.config_module_args) + with self.assertRaises(AnsibleExitJson) as exc: + set_module_args(self.config_module_args.copy()) self.module.main() - @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.arbitrary) + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_add(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['add'] = "name=unit_test_brige" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], True) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_add_already_exist(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleFailJson) as exc: module_args = self.config_module_args.copy() module_args['add'] = "name=unit_test_brige_exist" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['failed'], True) + self.assertEqual(result['msg'][0], 'failure: already have interface with such name') + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_remove(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['remove'] = "*A1" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], True) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_remove_no_id(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleFailJson) as exc: module_args = self.config_module_args.copy() module_args['remove'] = "*A2" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['failed'], True) + self.assertEqual(result['msg'][0], 'no such item (4)') + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.arbitrary) def test_api_cmd(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['cmd'] = "add name=unit_test_brige_arbitrary" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.arbitrary) def test_api_cmd_none_existing_cmd(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleFailJson) as exc: module_args = self.config_module_args.copy() module_args['cmd'] = "add NONE_EXIST=unit_test_brige_arbitrary" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['failed'], True) + self.assertEqual(result['msg'][0], 'no such command') + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_update(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['update'] = ".id=*A1 name=unit_test_brige" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], True) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_update_none_existing_id(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleFailJson) as exc: module_args = self.config_module_args.copy() module_args['update'] = ".id=*A2 name=unit_test_brige" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['failed'], True) + self.assertEqual(result['msg'][0], 'no such item (4)') + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_query(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['query'] = ".id name" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api) def test_api_query_missing_key(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['query'] = ".id other" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.select_where) def test_api_query_and_WHERE(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() module_args['query'] = ".id name WHERE name == dummy_bridge_A2" set_module_args(module_args) self.module.main() + result = exc.exception.args[0] + self.assertEqual(result['changed'], False) + @patch('ansible_collections.community.routeros.plugins.modules.api.ROS_api_module.api_add_path', new=fake_ros_api.select_where) def test_api_query_and_WHERE_no_cond(self): - with self.assertRaises(AnsibleExitJson): + with self.assertRaises(AnsibleExitJson) as exc: module_args = self.config_module_args.copy() - module_args['query'] = ".id name WHERE name =! dummy_bridge_A2" + module_args['query'] = ".id name WHERE name != dummy_bridge_A2" set_module_args(module_args) self.module.main() + + result = exc.exception.args[0] + self.assertEqual(result['changed'], False)