Merge pull request #28 from vyos/T8989_vyos_hostname_SIT_fix

T8989: vyos_hostname rework

4 files changed, 363 insertions, 25 deletions

diff --git a/tests/integration/targets/vyos_hostname/tests/httpapi/deleted.yaml b/tests/integration/targets/vyos_hostname/tests/httpapi/deleted.yaml
new file mode 100644
index 0000000..8ab278b
--- /dev/null
+++ b/tests/integration/targets/vyos_hostname/tests/httpapi/deleted.yaml
@@ -0,0 +1,39 @@
+---
+- debug:
+    msg: START vyos_hostname deleted integration tests on connection={{ ansible_connection }}
+
+- include_tasks: _remove_config.yaml
+- include_tasks: _populate_config.yaml
+
+- block:
+    - name: Delete hostname configuration
+      register: result
+      vyos.rest.vyos_hostname: &id001
+        state: deleted
+
+    - assert:
+        that:
+          - result.changed == true
+
+    - name: Gather after delete
+      register: gathered
+      vyos.rest.vyos_hostname:
+        state: gathered
+
+    - name: Assert hostname no longer the populated test value
+      assert:
+        that:
+          - gathered.gathered.hostname != "ansible-test-host"
+
+    - name: Delete hostname configuration (IDEMPOTENT)
+      register: result
+      vyos.rest.vyos_hostname: *id001
+
+    - name: Assert idempotent
+      assert:
+        that:
+          - result.changed == false
+          - result.commands == []
+
+  always:
+    - include_tasks: _remove_config.yaml
diff --git a/tests/integration/targets/vyos_hostname/tests/httpapi/overridden.yaml b/tests/integration/targets/vyos_hostname/tests/httpapi/overridden.yaml
index 671ac41..68c245c 100644
--- a/tests/integration/targets/vyos_hostname/tests/httpapi/overridden.yaml
+++ b/tests/integration/targets/vyos_hostname/tests/httpapi/overridden.yaml
@@ -1,27 +1,24 @@
 ---
 - debug:
-    msg: START vyos_interfaces overridden integration tests on connection={{ ansible_connection }}
+    msg: START vyos_hostname overridden integration tests on connection={{ ansible_connection }}
 
 - include_tasks: _remove_config.yaml
-- include_tasks: _populate_config.yaml
 
 - block:
-    - name: Override interface configuration
+    - name: Override hostname configuration
       register: result
-      vyos.rest.vyos_interfaces: &id001
+      vyos.rest.vyos_hostname: &id001
         config:
-          - name: eth0
-            description: Ansible overridden interface
-            mtu: 1400
+          hostname: ansible-overridden-host
         state: overridden
 
     - assert:
         that:
           - result.changed == true
 
-    - name: Override interface configuration (IDEMPOTENT)
+    - name: Override hostname configuration (IDEMPOTENT)
       register: result
-      vyos.rest.vyos_interfaces: *id001
+      vyos.rest.vyos_hostname: *id001
 
     - assert:
         that:
diff --git a/tests/integration/targets/vyos_hostname/tests/httpapi/rtt.yaml b/tests/integration/targets/vyos_hostname/tests/httpapi/rtt.yaml
index 74261f8..233f1ef 100644
--- a/tests/integration/targets/vyos_hostname/tests/httpapi/rtt.yaml
+++ b/tests/integration/targets/vyos_hostname/tests/httpapi/rtt.yaml
@@ -1,48 +1,41 @@
 ---
 - debug:
-    msg: START vyos_interfaces round trip integration tests on connection={{ ansible_connection }}
+    msg: START vyos_hostname round trip integration tests on connection={{ ansible_connection }}
 
 - include_tasks: _remove_config.yaml
 
 - block:
     - name: RTT - Apply base configuration
-      vyos.rest.vyos_interfaces:
+      vyos.rest.vyos_hostname:
         config:
-          - name: eth0
-            description: Ansible RTT interface
-            mtu: 1450
+          hostname: ansible-rtt-host
         state: merged
 
     - name: RTT - Gather configuration
       register: gathered
-      vyos.rest.vyos_interfaces:
+      vyos.rest.vyos_hostname:
         state: gathered
 
     - name: RTT - Assert gathered matches applied
       assert:
         that:
-          - gathered.gathered | selectattr('name', 'eq', 'eth0') | list | length == 1
-          - gathered.gathered | selectattr('name', 'eq', 'eth0') | map(attribute='description') | first == 'Ansible RTT interface'
-          - gathered.gathered | selectattr('name', 'eq', 'eth0') | map(attribute='mtu') | first == 1450
+          - gathered.gathered.hostname == "ansible-rtt-host"
 
     - name: RTT - Modify configuration
-      vyos.rest.vyos_interfaces:
+      vyos.rest.vyos_hostname:
         config:
-          - name: eth0
-            description: Ansible RTT modified
-            mtu: 1400
+          hostname: ansible-rtt-modified
         state: replaced
 
     - name: RTT - Gather modified configuration
       register: gathered2
-      vyos.rest.vyos_interfaces:
+      vyos.rest.vyos_hostname:
         state: gathered
 
     - name: RTT - Assert modification applied correctly
       assert:
         that:
-          - gathered2.gathered | selectattr('name', 'eq', 'eth0') | map(attribute='description') | first == 'Ansible RTT modified'
-          - gathered2.gathered | selectattr('name', 'eq', 'eth0') | map(attribute='mtu') | first == 1400
+          - gathered2.gathered.hostname == "ansible-rtt-modified"
 
   always:
     - include_tasks: _remove_config.yaml
diff --git a/tests/unit/modules/test_vyos_hostname.py b/tests/unit/modules/test_vyos_hostname.py
new file mode 100644
index 0000000..3a7d99d
--- /dev/null
+++ b/tests/unit/modules/test_vyos_hostname.py
@@ -0,0 +1,309 @@
+# -*- coding: utf-8 -*-
+from __future__ import absolute_import, division, print_function
+
+
+__metaclass__ = type
+
+import unittest
+
+from unittest.mock import MagicMock
+
+from ansible_collections.vyos.rest.plugins.modules.vyos_hostname import (
+    _BASE,
+    ARGUMENT_SPEC,
+    build_commands,
+    get_running_config,
+)
+
+
+class TestGetRunningConfig(unittest.TestCase):
+    def test_returns_current_hostname(self):
+        mock_vyos = MagicMock()
+        mock_vyos.get_value = MagicMock(return_value="vyos-core-01")
+        self.assertEqual(get_running_config(mock_vyos), "vyos-core-01")
+
+    def test_uses_get_value_not_get_config(self):
+        """Regression test for the confirmed architectural bug: this
+        module was previously calling a "showConfig"-equivalent
+        operation directly via a bare VyOSRestClient, appropriate for
+        config subtrees, not the "returnValue" operation VyOS provides
+        specifically for a single scalar leaf like this one. Confirms
+        get_running_config goes through VyOSModule.get_value, not
+        get_config."""
+        mock_vyos = MagicMock()
+        mock_vyos.get_value = MagicMock(return_value="vyos")
+        mock_vyos.get_config = MagicMock(return_value={})
+        get_running_config(mock_vyos)
+        mock_vyos.get_value.assert_called_once_with(_BASE)
+        mock_vyos.get_config.assert_not_called()
+
+
+class TestBuildCommands(unittest.TestCase):
+    def test_merged_sets_new_hostname(self):
+        cmds = build_commands({"hostname": "newhost"}, "vyos", "merged")
+        self.assertEqual(cmds, [("set", _BASE + ["newhost"])])
+
+    def test_merged_idempotent_when_already_correct(self):
+        cmds = build_commands({"hostname": "vyos"}, "vyos", "merged")
+        self.assertEqual(cmds, [])
+
+    def test_merged_noop_when_hostname_not_specified(self):
+        cmds = build_commands({}, "vyos", "merged")
+        self.assertEqual(cmds, [])
+
+    def test_deleted_with_existing_value(self):
+        cmds = build_commands({}, "somehost", "deleted")
+        self.assertEqual(cmds, [("delete", _BASE)])
+
+    def test_deleted_idempotent_when_already_empty(self):
+        cmds = build_commands({}, "", "deleted")
+        self.assertEqual(cmds, [])
+
+    def test_config_none_does_not_crash(self):
+        cmds = build_commands(None, "vyos", "merged")
+        self.assertEqual(cmds, [])
+
+
+class TestHostnameKeyAlwaysPresent(unittest.TestCase):
+    """Regression test for a real bug caught via integration testing:
+    "hostname" was conditionally omitted from the gathered/before/after
+    dict entirely when its value was empty (e.g. after deletion),
+    rather than being present with an empty value. This broke any
+    downstream access like gathered.gathered.hostname with an
+    AttributeError-equivalent ("object of type 'dict' has no attribute
+    'hostname'"), rather than a clean value comparison."""
+
+    def test_gathered_includes_hostname_key_when_empty(self):
+        import json
+        import sys
+
+        from unittest.mock import patch
+
+        from ansible.module_utils import basic
+
+        argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})]
+        captured = {}
+
+        def fake_exit_json(self_mod, **kwargs):
+            captured.update(kwargs)
+            raise SystemExit(0)
+
+        basic._ANSIBLE_ARGS = None
+        basic._PARSED_MODULE_ARGS = None
+        with patch.object(sys, "argv", argv), patch(
+            "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient",
+        ) as mock_client, patch(
+            "ansible.module_utils.basic.AnsibleModule.exit_json",
+            fake_exit_json,
+        ):
+            mock_client.return_value.retrieve_return_value.return_value = {"data": ""}
+            from ansible_collections.vyos.rest.plugins.modules import vyos_hostname
+
+            with self.assertRaises(SystemExit):
+                vyos_hostname.main()
+
+        self.assertIn("hostname", captured["gathered"])
+        self.assertEqual(captured["gathered"]["hostname"], "")
+
+
+class TestGatheredReturnsCommands(unittest.TestCase):
+    """Regression test for a bug caught during rework: the gathered
+    branch initially dropped commands=[] entirely, contradicting the
+    RETURN doc's own "returned: always" claim and regressing from the
+    original module's behavior."""
+
+    def test_gathered_includes_empty_commands(self):
+        import json
+        import sys
+
+        from unittest.mock import patch
+
+        from ansible.module_utils import basic
+
+        argv = [
+            "x",
+            json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}}),
+        ]
+        captured = {}
+
+        def fake_exit_json(self_mod, **kwargs):
+            captured.update(kwargs)
+            raise SystemExit(0)
+
+        basic._ANSIBLE_ARGS = None
+        basic._PARSED_MODULE_ARGS = None
+        with patch.object(sys, "argv", argv), patch(
+            "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient",
+        ) as mock_client, patch(
+            "ansible.module_utils.basic.AnsibleModule.exit_json",
+            fake_exit_json,
+        ):
+            mock_client.return_value.retrieve_return_value.return_value = {"data": "vyos"}
+            from ansible_collections.vyos.rest.plugins.modules import vyos_hostname
+
+            with self.assertRaises(SystemExit):
+                vyos_hostname.main()
+
+        self.assertIn("commands", captured)
+        self.assertEqual(captured["commands"], [])
+        self.assertEqual(captured["gathered"], {"hostname": "vyos"})
+
+
+class TestCollapsedStates(unittest.TestCase):
+    """replaced/overridden collapse onto merged for this single-value
+    resource -- there is nothing else to distinctly replace/override
+    when there is only one field. Confirmed via the argspec: no
+    separate branch exists for them in build_commands, matching the
+    module's own documented design decision."""
+
+    def test_argspec_declares_all_four_states(self):
+        self.assertEqual(
+            set(ARGUMENT_SPEC["state"]["choices"]),
+            {"merged", "replaced", "overridden", "deleted", "gathered"},
+        )
+
+    def test_no_rendered_or_parsed_states(self):
+        """Confirmed architectural fix: rendered/parsed are CLI-
+        collection concepts (offline command/config-text rendering)
+        that don't correspond to anything meaningful for a REST
+        transport, where the actual payload is a structured API call,
+        not a CLI line."""
+        self.assertNotIn("rendered", ARGUMENT_SPEC["state"]["choices"])
+        self.assertNotIn("parsed", ARGUMENT_SPEC["state"]["choices"])
+
+
+class TestArgumentSpecNoConnectionParams(unittest.TestCase):
+    """Confirmed architectural fix: module-level hostname/port/api_key/
+    timeout/verify_ssl params were a genuine outlier in this collection
+    -- every other module relies exclusively on the httpapi connection
+    plugin for transport/auth, never on module params."""
+
+    def test_no_connection_params_in_argspec(self):
+        for key in ("hostname", "port", "api_key", "timeout", "verify_ssl"):
+            self.assertNotIn(key, ARGUMENT_SPEC)
+
+
+class TestEmptyHostnameFailsExplicitly(unittest.TestCase):
+    """Confirmed real bug (Copilot): with state=merged, an empty
+    string for config.hostname previously produced a silent no-op --
+    build_commands' falsy check treated "" the same as "not
+    specified" -- even though hostname is declared required=True.
+    Ansible's own argspec validation only checks presence, not
+    non-emptiness, so this reached build_commands unnoticed. Confirmed
+    by direct reproduction before fixing; now fails explicitly instead.
+
+    In-process pattern (patch.object for sys.argv, matching the
+    established tests elsewhere in this file) rather than subprocess
+    isolation: confirmed the subprocess approach breaks under
+    ansible-test --docker's custom collection-loading machinery, which
+    a freshly spawned subprocess does not inherit.
+    """
+
+    def _run_main(self, config, state="merged"):
+        import json
+        import sys
+
+        from unittest.mock import patch
+
+        from ansible.module_utils import basic
+
+        args = {"state": state}
+        if config is not None:
+            args["config"] = config
+        argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": args})]
+        captured = {}
+
+        def fake_exit_json(self_mod, **kwargs):
+            captured.update(kwargs)
+            raise SystemExit(0)
+
+        def fake_fail_json(self_mod, **kwargs):
+            captured.update(kwargs)
+            captured["failed"] = True
+            raise SystemExit(1)
+
+        basic._ANSIBLE_ARGS = None
+        basic._PARSED_MODULE_ARGS = None
+        with patch.object(sys, "argv", argv), patch(
+            "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient",
+        ) as mock_client, patch(
+            "ansible.module_utils.basic.AnsibleModule.exit_json",
+            fake_exit_json,
+        ), patch(
+            "ansible.module_utils.basic.AnsibleModule.fail_json",
+            fake_fail_json,
+        ):
+            mock_client.return_value.retrieve_return_value.return_value = {
+                "data": "existing-host",
+            }
+            from ansible_collections.vyos.rest.plugins.modules import vyos_hostname
+
+            with self.assertRaises(SystemExit):
+                vyos_hostname.main()
+        return captured
+
+    def test_empty_hostname_fails_explicitly(self):
+        result = self._run_main({"hostname": ""})
+        self.assertTrue(result.get("failed"))
+        self.assertIn("non-empty", result.get("msg", ""))
+
+    def test_non_empty_hostname_still_works(self):
+        result = self._run_main({"hostname": "newhost"})
+        self.assertNotIn("failed", result)
+
+
+class TestCheckModeOmitsAfter(unittest.TestCase):
+    """Confirmed real bug (Copilot): check_mode returned after=have
+    even when commands was non-empty, misrepresenting the pre-change
+    state as if it were the post-change result. Now matches the
+    established convention across the rest of the collection
+    (confirmed against vyos_nat, vyos_ha, vyos_snmp_server,
+    vyos_ntp_global): omit "after" entirely in check mode.
+    """
+
+    def test_check_mode_omits_after(self):
+        import json
+        import sys
+
+        from unittest.mock import patch
+
+        from ansible.module_utils import basic
+
+        argv = [
+            "x",
+            json.dumps(
+                {
+                    "ANSIBLE_MODULE_ARGS": {
+                        "config": {"hostname": "newhost"},
+                        "state": "merged",
+                        "_ansible_check_mode": True,
+                    },
+                },
+            ),
+        ]
+        captured = {}
+
+        def fake_exit_json(self_mod, **kwargs):
+            captured.update(kwargs)
+            raise SystemExit(0)
+
+        basic._ANSIBLE_ARGS = None
+        basic._PARSED_MODULE_ARGS = None
+        with patch.object(sys, "argv", argv), patch(
+            "ansible_collections.vyos.rest.plugins.module_utils.vyos.VyOSRestClient",
+        ) as mock_client, patch(
+            "ansible.module_utils.basic.AnsibleModule.exit_json",
+            fake_exit_json,
+        ):
+            mock_client.return_value.retrieve_return_value.return_value = {"data": "oldhost"}
+            from ansible_collections.vyos.rest.plugins.modules import vyos_hostname
+
+            with self.assertRaises(SystemExit):
+                vyos_hostname.main()
+
+        self.assertTrue(captured.get("changed"))
+        self.assertNotIn("after", captured)
+
+
+if __name__ == "__main__":
+    unittest.main()