Skip to content

Commit

Permalink
ip dhcp-client: dhcp-options can be removed with value '' (#154)
Browse files Browse the repository at this point in the history 
* dhcp-options can be removed with value ''.

* Fix handling of disabled keys on creation.

* Fix typo.
  • Loading branch information
@felixfontein
felixfontein authored Mar 19, 2023
1 parent 071f742 commit 4329928
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 11 deletions.
3 changes: 3 additions & 0 deletions changelogs/fragments/154-ip-dhcp-client-dhcp-options.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
bugfixes:
- "api_info, api_modify - fix default and remove behavior for ``dhcp-options`` in path ``ip dhcp-client`` (https://github.com/ansible-collections/community.routeros/issues/148, https://github.com/ansible-collections/community.routeros/pull/154)."
- "api_modify - fix handling of disabled keys on creation (https://github.com/ansible-collections/community.routeros/pull/154)."
2 changes: 1 addition & 1 deletion plugins/module_utils/_api_data.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1314,7 +1314,7 @@ def join_path(path):
'add-default-route': KeyInfo(default=True),
'comment': KeyInfo(can_disable=True, remove_value=''),
'default-route-distance': KeyInfo(default=1),
'dhcp-options': KeyInfo(default='hostname,clientid'),
'dhcp-options': KeyInfo(default='hostname,clientid', can_disable=True, remove_value=''),
'disabled': KeyInfo(default=False),
'interface': KeyInfo(),
'script': KeyInfo(can_disable=True),
Expand Down
20 changes: 18 additions & 2 deletions plugins/modules/api_modify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,22 @@ def get_api_data(api_path, path_info):
return entries


def prepare_for_add(entry, path_info):
new_entry = {}
for k, v in entry.items():
if k.startswith('!'):
real_k = k[1:]
remove_value = path_info.fields[real_k].remove_value
if remove_value is not None:
k = real_k
v = remove_value
else:
if v is None:
v = path_info.fields[k].remove_value
new_entry[k] = v
return new_entry


def sync_list(module, api, path, path_info):
handle_absent_entries = module.params['handle_absent_entries']
handle_entries_content = module.params['handle_entries_content']
Expand Down Expand Up @@ -661,7 +677,7 @@ def match(current_entry):
)
for entry in create_list:
try:
entry['.id'] = api_path.add(**entry)
entry['.id'] = api_path.add(**prepare_for_add(entry, path_info))
except (LibRouterosError, UnicodeEncodeError) as e:
module.fail_json(
msg='Error while creating entry: {error}'.format(
Expand Down Expand Up @@ -850,7 +866,7 @@ def sync_with_primary_keys(module, api, path, path_info):
)
for entry in create_list:
try:
entry['.id'] = api_path.add(**entry)
entry['.id'] = api_path.add(**prepare_for_add(entry, path_info))
# Store ID for primary keys
pks = tuple(entry[primary_key] for primary_key in primary_keys)
id_by_key[pks] = entry['.id']
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/plugins/modules/fake_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ def str_return(self):
return repr(self.args)


def _normalize_entry(entry, path_info):
def _normalize_entry(entry, path_info, on_create=False):
for key, data in path_info.fields.items():
if key not in entry and data.default is not None and not data.can_disable:
if key not in entry and data.default is not None and (not data.can_disable or on_create):
entry[key] = data.default
if data.can_disable:
if key in entry and entry[key] in (None, data.remove_value):
Expand Down Expand Up @@ -188,7 +188,7 @@ def add(self, **kwargs):
'.id': id,
}
entry.update(kwargs)
_normalize_entry(entry, self._path_info)
_normalize_entry(entry, self._path_info, on_create=True)
self._values.append(entry)
return id

Expand Down
146 changes: 141 additions & 5 deletions tests/unit/plugins/modules/test_api_modify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,48 @@

START_IP_ADDRESS_OLD_DATA = massage_expected_result_data(START_IP_ADDRESS, ('ip', 'address'))

START_IP_DHCP_SEVER_LEASE = [
START_IP_DHCP_CLIENT = [
{
"!comment": None,
"!script": None,
".id": "*1",
"add-default-route": True,
"default-route-distance": 1,
"dhcp-options": "hostname,clientid",
"disabled": False,
"interface": "ether1",
"use-peer-dns": True,
"use-peer-ntp": True,
},
{
"!comment": None,
"!dhcp-options": None,
"!script": None,
".id": "*2",
"add-default-route": True,
"default-route-distance": 1,
"disabled": False,
"interface": "ether2",
"use-peer-dns": True,
"use-peer-ntp": True,
},
{
"!comment": None,
"!script": None,
".id": "*3",
"add-default-route": True,
"default-route-distance": 1,
"dhcp-options": "hostname",
"disabled": False,
"interface": "ether3",
"use-peer-dns": True,
"use-peer-ntp": True,
},
]

START_IP_DHCP_CLIENT_OLD_DATA = massage_expected_result_data(START_IP_DHCP_CLIENT, ('ip', 'dhcp-client'))

START_IP_DHCP_SERVER_LEASE = [
{
'.id': '*1',
'address': '192.168.88.2',
Expand Down Expand Up @@ -155,7 +196,7 @@
},
]

START_IP_DHCP_SEVER_LEASE_OLD_DATA = massage_expected_result_data(START_IP_DHCP_SEVER_LEASE, ('ip', 'dhcp-server', 'lease'))
START_IP_DHCP_SERVER_LEASE_OLD_DATA = massage_expected_result_data(START_IP_DHCP_SERVER_LEASE, ('ip', 'dhcp-server', 'lease'))

START_INTERFACE_LIST = [
{
Expand Down Expand Up @@ -1729,7 +1770,7 @@ def test_sync_primary_key_cru_reorder_check(self):
])

@patch('ansible_collections.community.routeros.plugins.modules.api_modify.compose_api_path',
new=create_fake_path(('ip', 'dhcp-server', 'lease'), START_IP_DHCP_SEVER_LEASE, read_only=True))
new=create_fake_path(('ip', 'dhcp-server', 'lease'), START_IP_DHCP_SERVER_LEASE, read_only=True))
def test_absent_value(self):
with self.assertRaises(AnsibleExitJson) as exc:
args = self.config_module_args.copy()
Expand Down Expand Up @@ -1769,8 +1810,103 @@ def test_absent_value(self):

result = exc.exception.args[0]
self.assertEqual(result['changed'], False)
self.assertEqual(result['old_data'], START_IP_DHCP_SEVER_LEASE_OLD_DATA)
self.assertEqual(result['new_data'], START_IP_DHCP_SEVER_LEASE_OLD_DATA)
self.assertEqual(result['old_data'], START_IP_DHCP_SERVER_LEASE_OLD_DATA)
self.assertEqual(result['new_data'], START_IP_DHCP_SERVER_LEASE_OLD_DATA)

@patch('ansible_collections.community.routeros.plugins.modules.api_modify.compose_api_path',
new=create_fake_path(('ip', 'dhcp-client'), START_IP_DHCP_CLIENT, read_only=True))
def test_default_remove_combination_idempotent(self):
with self.assertRaises(AnsibleExitJson) as exc:
args = self.config_module_args.copy()
args.update({
'path': 'ip dhcp-client',
'data': [
{
'interface': 'ether1',
},
{
'interface': 'ether2',
'dhcp-options': None,
},
{
'interface': 'ether3',
'dhcp-options': 'hostname',
},
],
'handle_absent_entries': 'remove',
'handle_entries_content': 'remove',
'ensure_order': True,
})
set_module_args(args)
self.module.main()

result = exc.exception.args[0]
self.assertEqual(result['changed'], False)
self.assertEqual(result['old_data'], START_IP_DHCP_CLIENT_OLD_DATA)
self.assertEqual(result['new_data'], START_IP_DHCP_CLIENT_OLD_DATA)

@patch('ansible_collections.community.routeros.plugins.modules.api_modify.compose_api_path',
new=create_fake_path(('ip', 'dhcp-client'), []))
def test_default_remove_combination_create(self):
with self.assertRaises(AnsibleExitJson) as exc:
args = self.config_module_args.copy()
args.update({
'path': 'ip dhcp-client',
'data': [
{
'interface': 'ether1',
},
{
'interface': 'ether2',
'dhcp-options': None,
},
{
'interface': 'ether3',
'dhcp-options': 'hostname',
},
],
'handle_absent_entries': 'remove',
'handle_entries_content': 'remove',
'ensure_order': True,
})
set_module_args(args)
self.module.main()

result = exc.exception.args[0]
self.assertEqual(result['changed'], True)
self.assertEqual(result['old_data'], [])
self.assertEqual(result['new_data'], [
{
".id": "*NEW1",
"add-default-route": True,
"default-route-distance": 1,
"dhcp-options": "hostname,clientid",
"disabled": False,
"interface": "ether1",
"use-peer-dns": True,
"use-peer-ntp": True,
},
{
# "!dhcp-options": None,
".id": "*NEW2",
"add-default-route": True,
"default-route-distance": 1,
"disabled": False,
"interface": "ether2",
"use-peer-dns": True,
"use-peer-ntp": True,
},
{
".id": "*NEW3",
"add-default-route": True,
"default-route-distance": 1,
"dhcp-options": "hostname",
"disabled": False,
"interface": "ether3",
"use-peer-dns": True,
"use-peer-ntp": True,
},
])

@patch('ansible_collections.community.routeros.plugins.modules.api_modify.compose_api_path',
new=create_fake_path(('interface', 'list'), START_INTERFACE_LIST, read_only=True))
Expand Down

0 comments on commit 4329928

Please sign in to comment.