ip dhcp-client: dhcp-options can be removed with value '' (#154)

* dhcp-options can be removed with value ''.

* Fix handling of disabled keys on creation.

* Fix typo.

changelogs/fragments/154-ip-dhcp-client-dhcp-options.yml

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

plugins/module_utils/_api_data.py

@@ -1314,7 +1314,7 @@ PATHS = {
             '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),

plugins/modules/api_modify.py

@@ -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']
@@ -661,7 +677,7 @@ def sync_list(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))
             except (LibRouterosError, UnicodeEncodeError) as e:
                 module.fail_json(
                     msg='Error while creating entry: {error}'.format(
@@ -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']

tests/unit/plugins/modules/fake_api.py

@@ -118,9 +118,9 @@ class Or(object):
         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):
@@ -188,7 +188,7 @@ class Path(object):
             '.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
 

tests/unit/plugins/modules/test_api_modify.py

@@ -89,7 +89,48 @@ START_IP_ADDRESS = [
 
 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',
@@ -155,7 +196,7 @@ START_IP_DHCP_SEVER_LEASE = [
     },
 ]
 
-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 = [
     {
@@ -1729,7 +1770,7 @@ class TestRouterosApiModifyModule(ModuleTestCase):
         ])
 
     @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()
@@ -1769,8 +1810,103 @@ class TestRouterosApiModifyModule(ModuleTestCase):
 
         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['old_data'], START_IP_DHCP_SERVER_LEASE_OLD_DATA)
-        self.assertEqual(result['new_data'], START_IP_DHCP_SEVER_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))