Allow api module to fail (#39)

* Allow api module to fail.

* Improve error handling.

* Fix api unit tests.

* Add very basic tests of return values.

* Update api.py

fix ignoring the Fail task if we get TrapError

* Do not mangle fail result, and adjust tests.

* Improve changelog fragment.

* Reclassify changelog fragment as minor_changes, improve text.

* Mark changelog as 'breaking change'.

Co-authored-by: Nikolay Dachev <nikolay@dachev.info>
This commit is contained in:
Felix Fontein 2021-07-11 15:53:22 +02:00 committed by GitHub
parent df88a7ec99
commit 69682054e1
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 94 additions and 61 deletions

View file

@ -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).

View file

@ -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)

View file

@ -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)