From ef3ded9ace9d7c650e15104698acdf629c57f797 Mon Sep 17 00:00:00 2001 From: omnom62 Date: Sun, 23 Aug 2026 08:39:18 +1000 Subject: T8989: vyos_hostname rework --- README.md | 5 +- changelogs/fragments/t8989_hostname_rework.yml | 3 + docs/vyos.rest.vyos_hostname_module.rst | 152 +---------- plugins/module_utils/vyos.py | 102 ++----- plugins/modules/vyos_hostname.py | 294 ++++++++------------- .../vyos_hostname/tests/httpapi/deleted.yaml | 39 +++ .../vyos_hostname/tests/httpapi/overridden.yaml | 15 +- .../targets/vyos_hostname/tests/httpapi/rtt.yaml | 25 +- tests/unit/modules/test_vyos_hostname.py | 179 +++++++++++++ 9 files changed, 377 insertions(+), 437 deletions(-) create mode 100644 changelogs/fragments/t8989_hostname_rework.yml create mode 100644 tests/integration/targets/vyos_hostname/tests/httpapi/deleted.yaml create mode 100644 tests/unit/modules/test_vyos_hostname.py diff --git a/README.md b/README.md index 50d4aad..5d33329 100644 --- a/README.md +++ b/README.md @@ -92,8 +92,7 @@ Name | Description [vyos.rest.vyos_l3_interfaces](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_l3_interfaces_module.rst)|Manage L3 interface configuration on VyOS devices via REST API. [vyos.rest.vyos_lag_interfaces](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_lag_interfaces_module.rst)|Manage LAG interface configuration on VyOS devices via REST API. [vyos.rest.vyos_lldp_global](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_lldp_global_module.rst)|Manage LLDP global configuration on VyOS via REST API. -[vyos.rest.vyos_lldp_interfaces](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_lldp_int -erfaces_module.rst)|Manage LLDP interface configuration on VyOS devices via REST API. +[vyos.rest.vyos_lldp_interfaces](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_lldp_interfaces_module.rst)|Manage LLDP interface configuration on VyOS devices via REST API. [vyos.rest.vyos_logging_global](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_logging_global_module.rst)|Manage syslog configuration on VyOS devices using REST API [vyos.rest.vyos_nat](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_nat_module.rst)|Manage NAT configuration on VyOS devices using REST API [vyos.rest.vyos_ntp_global](https://github.com/vyos/vyos.rest/blob/main/docs/vyos.rest.vyos_ntp_global_module.rst)|Manage NTP configuration on VyOS devices using REST API @@ -134,5 +133,3 @@ automation. | Connection plugin | `ansible.netcommon.network_cli` | `ansible.netcommon.httpapi` | | VyOS requirement | Any | VyOS 1.3+ with REST API enabled | | Atomic commits | Per-command | Batch (single commit) | - - diff --git a/changelogs/fragments/t8989_hostname_rework.yml b/changelogs/fragments/t8989_hostname_rework.yml new file mode 100644 index 0000000..cc16dbd --- /dev/null +++ b/changelogs/fragments/t8989_hostname_rework.yml @@ -0,0 +1,3 @@ +--- +bug_fixes: + - vyos_hostname - Added methoed to vyos helper, fixed the bugs in the module and in UAT/SIT tests. diff --git a/docs/vyos.rest.vyos_hostname_module.rst b/docs/vyos.rest.vyos_hostname_module.rst index c8ea1c5..7e4e37e 100644 --- a/docs/vyos.rest.vyos_hostname_module.rst +++ b/docs/vyos.rest.vyos_hostname_module.rst @@ -17,18 +17,10 @@ Version added: 1.0.0 Synopsis -------- -- Manages the ``set system host-name`` configuration on a VyOS device using the HTTPS REST API. -- Mirrors the behaviour of ``vyos.vyos.vyos_hostname`` but uses the HTTP API instead of SSH/network_cli. -- The states ``replaced``, ``overridden`` behave identically to ``merged`` for this single-value resource. +- Manages the ``system host-name`` configuration on a VyOS device using the HTTPS REST API. -Requirements ------------- -The below requirements are needed on the host that executes this module. - -- VyOS 1.3+ - Parameters ---------- @@ -41,21 +33,6 @@ Parameters Choices/Defaults Comments - - -
- api_key - -
- string -
- - - - -
API key configured on the device.
- -
@@ -89,53 +66,6 @@ Parameters - - -
- hostname - -
- string -
- - - - -
IP address or FQDN of the VyOS device (not needed with httpapi inventory).
- - - - -
- port - -
- integer -
- - - Default:
443
- - -
HTTPS port for the REST API.
- - - - -
- running_config - -
- string -
- - - - -
Used only with state parsed.
-
The value should be the output of show configuration commands | grep host-name from the device.
- -
@@ -152,53 +82,13 @@ Parameters
  • overridden
  • deleted
  • gathered
    • -
  • rendered
    • -
  • parsed
    • merged - Ensure the hostname is set to the value in config.
    -
    replaced - Identical to merged for this single-value resource.
    -
    overridden - Identical to merged for this single-value resource.
    -
    deleted - Remove the configured hostname (resets to default).
    -
    gathered - Read the current hostname from the device and return it in gathered without making changes.
    -
    rendered - Return the CLI commands for the given config without connecting to the device.
    -
    parsed - Parse the running_config string and return structured data without connecting to the device.
    - - - - -
    - timeout - -
    - integer -
    - - - Default:
    30
    - - -
    Request timeout in seconds.
    - - - - -
    - verify_ssl - -
    - boolean -
    - - - - - -
    Validate the device's TLS certificate.
    +
    replaced and overridden behave identically to merged for this single-value resource -- there is nothing else to distinctly replace or override when there is only one field.
    +
    deleted - Remove the configured hostname.
    +
    gathered - Read the current hostname from the device without making changes.
    @@ -226,32 +116,14 @@ Examples hostname: vyos-core-01 state: merged - - name: Replace hostname - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-02 - state: replaced - - name: Gather current hostname vyos.rest.vyos_hostname: state: gathered - register: result - name: Delete hostname configuration vyos.rest.vyos_hostname: state: deleted - - name: Render commands without connecting - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-01 - state: rendered - - - name: Parse running config - vyos.rest.vyos_hostname: - running_config: "set system host-name 'vyos'" - state: parsed - Return Values @@ -307,7 +179,7 @@ Common return values are documented `here always -
    REST API commands dispatched.
    +
    List of API command tuples sent to the device.

    @@ -329,30 +201,30 @@ Common return values are documented `here
    - parsed + response
    dictionary
    - when state is parsed + when changes are applied -
    Structured data parsed from running_config (state=parsed only).
    +
    Raw API response.

    - rendered + saved
    - list + boolean
    - when state is rendered + when changes are applied -
    CLI commands for the provided config (state=rendered only).
    +
    Whether the config was saved after changes.

    diff --git a/plugins/module_utils/vyos.py b/plugins/module_utils/vyos.py index a37a0c2..bdbc2c4 100644 --- a/plugins/module_utils/vyos.py +++ b/plugins/module_utils/vyos.py @@ -18,85 +18,6 @@ from ansible_collections.vyos.rest.plugins.module_utils.vyos_rest import ( ) -# --------------------------------------------------------------------------- -# Legacy dynamic config utilities (used by Wave 1-3 modules) -# --------------------------------------------------------------------------- - - -def _kebab_to_snake(s): - """Convert kebab-case string to snake_case.""" - return s.replace("-", "_") - - -def _snake_to_kebab(s): - """Convert snake_case string to kebab-case.""" - return s.replace("_", "-") - - -def normalize(raw): - """Recursively normalize an API response dict to snake_case keys.""" - if isinstance(raw, dict): - return {_kebab_to_snake(k): normalize(v) for k, v in raw.items()} - if isinstance(raw, list): - return [normalize(v) for v in raw] - return raw - - -def denormalize_path(path): - """Convert a snake_case path list to kebab-case for the API.""" - return [_snake_to_kebab(p) for p in path] - - -def _diff_value(want_val, have_val, path, cmds, delete_missing): - if isinstance(want_val, dict): - if not want_val: - if have_val is None: - cmds.append(("set", denormalize_path(path))) - else: - have_dict = have_val if isinstance(have_val, dict) else {} - _diff_dict(want_val, have_dict, path, cmds, delete_missing) - elif isinstance(want_val, list): - have_set = set(have_val) if isinstance(have_val, list) else set() - for item in want_val: - if item not in have_set: - cmds.append(("set", denormalize_path(path + [str(item)]))) - if delete_missing: - want_set = set(str(i) for i in want_val) - for item in have_val or []: - if str(item) not in want_set: - cmds.append(("delete", denormalize_path(path + [str(item)]))) - else: - if want_val != have_val: - cmds.append(("set", denormalize_path(path + [str(want_val)]))) - - -def _diff_dict(want, have, path, cmds, delete_missing): - for key, want_val in want.items(): - _diff_value(want_val, have.get(key), path + [key], cmds, delete_missing) - if delete_missing: - for key in have: - if key not in want: - cmds.append(("delete", denormalize_path(path + [key]))) - - -def diff_configs(want, have, base_path, delete_missing=False): - """Diff two normalized config dicts and return API command tuples. - - Args: - want (dict): Desired configuration (snake_case keys). - have (dict): Current configuration (snake_case keys). - base_path (list): Base API path for commands. - delete_missing (bool): Generate delete commands for keys in - ``have`` absent from ``want``. - - Returns: - list: Tuples of ``("set", path)`` or ``("delete", path)``. - """ - cmds = [] - _diff_dict(want, have, base_path, cmds, delete_missing) - return cmds - - # --------------------------------------------------------------------------- # Generic dict diff engine (used by Wave 4+ modules) # @@ -229,7 +150,10 @@ def cast_by_spec(entry, options): continue spec_type = spec.get("type") if spec_type == "int": - entry[key] = int(entry[key]) + val = entry[key] + if isinstance(val, list): + val = val[0] if val else None + entry[key] = int(val) if val is not None else None elif spec_type == "dict": cast_by_spec(entry[key], spec.get("options")) elif spec_type == "list": @@ -380,6 +304,24 @@ class VyOSModule: except VyOSRestError: return {} + def get_value(self, path): + """Retrieve a single scalar leaf value at *path*. + + Uses VyOS's dedicated "returnValue" retrieve operation -- + genuinely distinct from get_config's "showConfig" operation, + which returns a config subtree rather than a single value. + Appropriate for a plain leafNode (e.g. "system host-name"), + not a container. + + Errors genuinely propagate rather than being swallowed into a + misleading "absent" result: a transient failure here must not + be indistinguishable from the value legitimately being unset, + since a module could otherwise decide to overwrite a value + that's actually already correct. + """ + result = self._client.retrieve_return_value(path) + return result.get("data") or "" + def apply_commands(self, commands): if not commands: return [] diff --git a/plugins/modules/vyos_hostname.py b/plugins/modules/vyos_hostname.py index 86444ad..3218333 100644 --- a/plugins/modules/vyos_hostname.py +++ b/plugins/modules/vyos_hostname.py @@ -12,83 +12,51 @@ DOCUMENTATION = r""" module: vyos_hostname short_description: Manage the system hostname on a VyOS device via the REST API. description: - - Manages the C(set system host-name) configuration on a VyOS device - using the HTTPS REST API. - - Mirrors the behaviour of C(vyos.vyos.vyos_hostname) but uses the HTTP - API instead of SSH/network_cli. - - The states C(replaced), C(overridden) behave identically to C(merged) - for this single-value resource. + - Manages the C(system host-name) configuration on a VyOS device using the HTTPS REST API. version_added: "1.0.0" author: - VyOS Community (@vyos) options: config: - description: - - Hostname configuration. + description: Hostname configuration. type: dict suboptions: hostname: - description: - - System hostname (max 63 characters, no underscores). + description: System hostname (max 63 characters, no underscores). type: str required: true - running_config: - description: - - Used only with state C(parsed). - - The value should be the output of - B(show configuration commands | grep host-name) from the device. - type: str state: description: - C(merged) - Ensure the hostname is set to the value in I(config). - - C(replaced) - Identical to C(merged) for this single-value resource. - - C(overridden) - Identical to C(merged) for this single-value resource. - - C(deleted) - Remove the configured hostname (resets to default). - - C(gathered) - Read the current hostname from the device and return it - in I(gathered) without making changes. - - C(rendered) - Return the CLI commands for the given config without - connecting to the device. - - C(parsed) - Parse the C(running_config) string and return structured - data without connecting to the device. + - >- + C(replaced) and C(overridden) behave identically to C(merged) for + this single-value resource -- there is nothing else to distinctly + replace or override when there is only one field. + - C(deleted) - Remove the configured hostname. + - C(gathered) - Read the current hostname from the device without making changes. type: str - choices: - - merged - - replaced - - overridden - - deleted - - gathered - - rendered - - parsed + choices: [merged, replaced, overridden, deleted, gathered] default: merged - hostname: - description: - - IP address or FQDN of the VyOS device (not needed with httpapi inventory). - type: str - port: - description: - - HTTPS port for the REST API. - type: int - default: 443 - api_key: - description: - - API key configured on the device. - type: str - timeout: - description: - - Request timeout in seconds. - type: int - default: 30 - verify_ssl: - description: - - Validate the device's TLS certificate. - type: bool - default: false -requirements: - - VyOS 1.3+ seealso: - module: vyos.vyos.vyos_hostname """ +EXAMPLES = r""" +- name: Set hostname + vyos.rest.vyos_hostname: + config: + hostname: vyos-core-01 + state: merged + +- name: Gather current hostname + vyos.rest.vyos_hostname: + state: gathered + +- name: Delete hostname configuration + vyos.rest.vyos_hostname: + state: deleted +""" + RETURN = r""" before: description: Configuration on the device before the module ran. @@ -102,173 +70,123 @@ gathered: description: Hostname read from the device (state=gathered only). returned: when state is gathered type: dict -rendered: - description: CLI commands for the provided config (state=rendered only). - returned: when state is rendered - type: list -parsed: - description: Structured data parsed from running_config (state=parsed only). - returned: when state is parsed - type: dict commands: - description: REST API commands dispatched. + description: List of API command tuples sent to the device. returned: always type: list +saved: + description: Whether the config was saved after changes. + returned: when changes are applied + type: bool +response: + description: Raw API response. + returned: when changes are applied + type: dict """ -EXAMPLES = r""" -- name: Set hostname - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-01 - state: merged - -- name: Replace hostname - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-02 - state: replaced - -- name: Gather current hostname - vyos.rest.vyos_hostname: - state: gathered - register: result - -- name: Delete hostname configuration - vyos.rest.vyos_hostname: - state: deleted - -- name: Render commands without connecting - vyos.rest.vyos_hostname: - config: - hostname: vyos-core-01 - state: rendered - -- name: Parse running config - vyos.rest.vyos_hostname: - running_config: "set system host-name 'vyos'" - state: parsed -""" - -import re - from ansible.module_utils.basic import AnsibleModule -from ansible_collections.vyos.rest.plugins.module_utils.vyos_rest import ( - VYOS_REST_CONNECTION_ARGSPEC, - VyOSRestClient, +from ansible_collections.vyos.rest.plugins.module_utils.vyos import ( + VyOSModule, VyOSRestError, ) -_PATH = ["system", "host-name"] +_BASE = ["system", "host-name"] -def _get_hostname(client): - try: - result = client.retrieve_return_value(_PATH) - return result.get("data", "") - except VyOSRestError: - return "" +def get_running_config(vyos): + """ "system host-name" is a plain leafNode, not a container -- a + single scalar value, not a config subtree. get_value (VyOS's + "returnValue" retrieve operation) is the correct fetch for this, + distinct from get_config's "showConfig" operation used everywhere + else in this collection for genuine nested config sections. + """ + return vyos.get_value(_BASE) -def _parse_hostname(running_config): - """Parse hostname from 'show configuration commands | grep host-name' output.""" - match = re.search(r"host-name\s+['\"]?(\S+?)['\"]?\s*$", running_config, re.M) - return match.group(1) if match else "" +def build_commands(config, current, state): + if state == "deleted": + return [("delete", _BASE)] if current else [] + desired = (config or {}).get("hostname") + if not desired or desired == current: + return [] + return [("set", _BASE + [desired])] -def main(): - argument_spec = dict( - config=dict( - type="dict", - options=dict( - hostname=dict(type="str", required=True), - ), - ), - running_config=dict(type="str"), - state=dict( - type="str", - default="merged", - choices=[ - "merged", - "replaced", - "overridden", - "deleted", - "gathered", - "rendered", - "parsed", - ], + +ARGUMENT_SPEC = dict( + config=dict( + type="dict", + options=dict( + hostname=dict(type="str", required=True), ), - ) - argument_spec.update(VYOS_REST_CONNECTION_ARGSPEC) + ), + state=dict( + type="str", + default="merged", + choices=["merged", "replaced", "overridden", "deleted", "gathered"], + ), +) + +def main(): module = AnsibleModule( - argument_spec=argument_spec, - mutually_exclusive=[["config", "running_config"]], + ARGUMENT_SPEC, required_if=[ ("state", "merged", ["config"]), ("state", "replaced", ["config"]), ("state", "overridden", ["config"]), - ("state", "rendered", ["config"]), - ("state", "parsed", ["running_config"]), ], supports_check_mode=True, ) + vyos = VyOSModule(module) + # Collapsed states: replaced/overridden are identical to merged for + # this single-value resource -- there is nothing else to distinctly + # replace or override when there's only one field. state = module.params["state"] - - # rendered — offline, no device connection needed - if state == "rendered": - hostname = module.params["config"]["hostname"] - module.exit_json( - rendered=["set system host-name '{h}'".format(h=hostname)], - commands=[], - ) - - # parsed — offline, no device connection needed - if state == "parsed": - hostname = _parse_hostname(module.params["running_config"] or "") - module.exit_json( - parsed={"hostname": hostname}, - commands=[], - ) - - # collapsed states — replaced and overridden are identical to merged if state in ("replaced", "overridden"): state = "merged" - client = VyOSRestClient(module) - commands = [] - changed = False - - current = _get_hostname(client) - before = {"hostname": current} + config = module.params.get("config") or {} - if state == "gathered": - module.exit_json(changed=False, gathered=before, before=before, commands=[]) + try: + current = get_running_config(vyos) + except VyOSRestError as exc: + module.fail_json(msg="failed to read current hostname: {e}".format(e=str(exc))) - if module.check_mode: - module.exit_json(changed=True, before=before, commands=["(check mode)"]) + have = {"hostname": current} - try: - if state == "merged": - desired = module.params["config"]["hostname"] - if current != desired: - client.configure_set(_PATH, desired) - commands.append("set system host-name '{h}'".format(h=desired)) - changed = True + if state == "gathered": + module.exit_json(changed=False, gathered=have, commands=[]) - elif state == "deleted": - if current: - client.configure_delete(_PATH) - commands.append("delete system host-name") - changed = True + commands = build_commands(config, current, state) - except VyOSRestError as exc: - module.fail_json(msg=str(exc)) + if module.check_mode: + module.exit_json(changed=bool(commands), commands=commands, before=have, after=have) + + if commands: + response = vyos.apply_commands(commands) + saved = vyos.save_config() + try: + after_current = get_running_config(vyos) + except VyOSRestError as exc: + module.fail_json( + msg="hostname change applied but failed to read back result: {e}".format( + e=str(exc), + ), + ) + after = {"hostname": after_current} + module.exit_json( + changed=True, + before=have, + after=after, + commands=commands, + saved=saved, + response=response, + ) - after = {"hostname": _get_hostname(client)} if changed else before - module.exit_json(changed=changed, before=before, after=after, commands=commands) + module.exit_json(changed=False, before=have, after=have, commands=[]) if __name__ == "__main__": 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..f5f40b3 --- /dev/null +++ b/tests/unit/modules/test_vyos_hostname.py @@ -0,0 +1,179 @@ +# -*- 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 + + sys.argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] + captured = {} + + def fake_exit_json(self_mod, **kwargs): + captured.update(kwargs) + raise SystemExit(0) + + with 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 + + sys.argv = [ + "x", + json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}}), + ] + captured = {} + + def fake_exit_json(self_mod, **kwargs): + captured.update(kwargs) + raise SystemExit(0) + + with 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) + + +if __name__ == "__main__": + unittest.main() -- cgit v1.2.3 From dedf5a00c69465b4ed067882b135822e316dee10 Mon Sep 17 00:00:00 2001 From: omnom62 Date: Sun, 23 Aug 2026 08:43:00 +1000 Subject: T8989: vyos_hostname rework --- changelogs/fragments/t8989_hostname_rework.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/t8989_hostname_rework.yml b/changelogs/fragments/t8989_hostname_rework.yml index cc16dbd..5b52d06 100644 --- a/changelogs/fragments/t8989_hostname_rework.yml +++ b/changelogs/fragments/t8989_hostname_rework.yml @@ -1,3 +1,3 @@ --- -bug_fixes: +bugfixes: - vyos_hostname - Added methoed to vyos helper, fixed the bugs in the module and in UAT/SIT tests. -- cgit v1.2.3 From 63de800cc7856e31253d9d5b4010f42263cf8b76 Mon Sep 17 00:00:00 2001 From: omnom62 <75066712+omnom62@users.noreply.github.com> Date: Mon, 24 Aug 2026 06:21:40 +1000 Subject: T8989: Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- changelogs/fragments/t8989_hostname_rework.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/t8989_hostname_rework.yml b/changelogs/fragments/t8989_hostname_rework.yml index 5b52d06..6e1db7c 100644 --- a/changelogs/fragments/t8989_hostname_rework.yml +++ b/changelogs/fragments/t8989_hostname_rework.yml @@ -1,3 +1,3 @@ --- bugfixes: - - vyos_hostname - Added methoed to vyos helper, fixed the bugs in the module and in UAT/SIT tests. + - vyos_hostname - Added method to VyOS helper; fixed bugs in the module and in UAT/SIT tests. -- cgit v1.2.3 From da624a86b8067ca730985d77c99922d08c6a8aa9 Mon Sep 17 00:00:00 2001 From: omnom62 Date: Mon, 14 Sep 2026 19:03:19 +1000 Subject: T8989: vyos_hostname AI comments fixed --- plugins/modules/vyos_hostname.py | 7 +- tests/unit/modules/test_vyos_hostname.py | 138 ++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/plugins/modules/vyos_hostname.py b/plugins/modules/vyos_hostname.py index 3218333..b7bdd50 100644 --- a/plugins/modules/vyos_hostname.py +++ b/plugins/modules/vyos_hostname.py @@ -149,6 +149,8 @@ def main(): state = "merged" config = module.params.get("config") or {} + if state == "merged" and not config.get("hostname"): + module.fail_json(msg="config.hostname must be a non-empty string") try: current = get_running_config(vyos) @@ -163,7 +165,10 @@ def main(): commands = build_commands(config, current, state) if module.check_mode: - module.exit_json(changed=bool(commands), commands=commands, before=have, after=have) + # Matches the established convention across the rest of the + # collection: omit "after" entirely in check mode, rather than + # reporting have as if it were the post-change state. + module.exit_json(changed=bool(commands), commands=commands, before=have) if commands: response = vyos.apply_commands(commands) diff --git a/tests/unit/modules/test_vyos_hostname.py b/tests/unit/modules/test_vyos_hostname.py index f5f40b3..713c0ff 100644 --- a/tests/unit/modules/test_vyos_hostname.py +++ b/tests/unit/modules/test_vyos_hostname.py @@ -79,14 +79,18 @@ class TestHostnameKeyAlwaysPresent(unittest.TestCase): from unittest.mock import patch - sys.argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] + import ansible.module_utils.basic as basic + + argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] captured = {} def fake_exit_json(self_mod, **kwargs): captured.update(kwargs) raise SystemExit(0) - with patch( + 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", @@ -114,7 +118,9 @@ class TestGatheredReturnsCommands(unittest.TestCase): from unittest.mock import patch - sys.argv = [ + import ansible.module_utils.basic as basic + + argv = [ "x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}}), ] @@ -124,7 +130,9 @@ class TestGatheredReturnsCommands(unittest.TestCase): captured.update(kwargs) raise SystemExit(0) - with patch( + 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", @@ -175,5 +183,127 @@ class TestArgumentSpecNoConnectionParams(unittest.TestCase): 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 + + import ansible.module_utils.basic as 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 + + import ansible.module_utils.basic as 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() -- cgit v1.2.3 From 807c2dcbc4c1f36236e0d7945699e6169ebbc78b Mon Sep 17 00:00:00 2001 From: omnom62 Date: Mon, 14 Sep 2026 20:59:53 +1000 Subject: T8989: vyos_hostname AI comments fixed --- tests/unit/modules/test_vyos_hostname.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/modules/test_vyos_hostname.py b/tests/unit/modules/test_vyos_hostname.py index 713c0ff..3a7d99d 100644 --- a/tests/unit/modules/test_vyos_hostname.py +++ b/tests/unit/modules/test_vyos_hostname.py @@ -79,7 +79,7 @@ class TestHostnameKeyAlwaysPresent(unittest.TestCase): from unittest.mock import patch - import ansible.module_utils.basic as basic + from ansible.module_utils import basic argv = ["x", json.dumps({"ANSIBLE_MODULE_ARGS": {"state": "gathered"}})] captured = {} @@ -118,7 +118,7 @@ class TestGatheredReturnsCommands(unittest.TestCase): from unittest.mock import patch - import ansible.module_utils.basic as basic + from ansible.module_utils import basic argv = [ "x", @@ -205,7 +205,7 @@ class TestEmptyHostnameFailsExplicitly(unittest.TestCase): from unittest.mock import patch - import ansible.module_utils.basic as basic + from ansible.module_utils import basic args = {"state": state} if config is not None: @@ -267,7 +267,7 @@ class TestCheckModeOmitsAfter(unittest.TestCase): from unittest.mock import patch - import ansible.module_utils.basic as basic + from ansible.module_utils import basic argv = [ "x", -- cgit v1.2.3