Fix device_role -> role and add testing for Netbox 3.6 (#1066)

* Add testing for v3.6 of Netbox.
* Add custom logic to account for device_role -> role for Netbox 3.6+ but can be removed eventually.
* Remove update_vc_child as query_param if statement as that is handled further down in code.

Author: FragmentedPacket

.github/workflows/main.yml

@@ -82,6 +82,8 @@ jobs:
             NETBOX_DOCKER_VERSION: 2.5.3
           - VERSION: "v3.5"
             NETBOX_DOCKER_VERSION: 2.6.1
+          - VERSION: "v3.6"
+            NETBOX_DOCKER_VERSION: 2.7.0
         # If we want to integration test wiht all supported Python:
         #python-version: ["3.9", "3.10", "3.11"]
 

hacking/local-test.sh

@@ -9,5 +9,5 @@
 ansible-galaxy collection install netbox-netbox-*.tar.gz -p .
 
 # You can now cd into the installed version and run tests
-(cd ansible_collections/netbox/netbox/ && ansible-test units -v --python 3.6 && ansible-test sanity --requirements -v --python 3.6 --skip-test pep8 plugins/)
+(cd ansible_collections/netbox/netbox/ && ansible-test units -v --python 3.10 && ansible-test sanity --requirements -v --python 3.10 --skip-test pep8 plugins/)
 rm -rf ansible_collections

plugins/module_utils/netbox_dcim.py

@@ -211,19 +211,15 @@ def run(self):
             )
 
         # This is logic to handle interfaces on a VC
-        if self.endpoint == "interfaces":
-            if self.nb_object:
-                device = self.nb.dcim.devices.get(self.nb_object.device.id)
-                if (
-                    device["virtual_chassis"]
-                    and self.nb_object.device.id != self.data["device"]
-                ):
-                    if self.module.params.get("update_vc_child"):
-                        data["device"] = self.nb_object.device.id
-                    else:
-                        self._handle_errors(
-                            msg="Must set update_vc_child to True to allow child device interface modification"
-                        )
+        if self.endpoint == "interfaces" and self.nb_object:
+            child = self.nb.dcim.devices.get(self.nb_object.device.id)
+            if child["virtual_chassis"] and child.id != data["device"]:
+                if self.module.params.get("update_vc_child"):
+                    data["device"] = child.id
+                else:
+                    self._handle_errors(
+                        msg="Must set update_vc_child to True to allow child device interface modification"
+                    )
 
         if self.state == "present":
             self._ensure_object_exists(nb_endpoint, endpoint_name, name, data)

plugins/module_utils/netbox_utils.py

@@ -562,6 +562,7 @@
     "cluster_type": "type",
     "cluster_group": "group",
     "contact_group": "group",
+    "device_role": "role",
     "fhrp_group": "group",
     "inventory_item_role": "role",
     "parent_contact_group": "parent",
@@ -819,9 +820,15 @@ def _convert_identical_keys(self, data):
         if self._version_check_greater(self.version, "2.7", greater_or_equal=True):
             if data.get("form_factor"):
                 temp_dict["type"] = data.pop("form_factor")
+
         for key in data:
             if self.endpoint == "power_panels" and key == "rack_group":
                 temp_dict[key] = data[key]
+            # TODO: Remove this once the lowest supported Netbox version is 3.6 or greater as we can use default logic of CONVERT_KEYS moving forward.
+            elif key == "device_role" and not self._version_check_greater(
+                self.version, "3.6", greater_or_equal=True
+            ):
+                temp_dict[key] = data[key]
             elif key in CONVERT_KEYS:
                 # This will keep the original key for keys in list, but also convert it.
                 if key in ("assigned_object", "scope"):
@@ -855,19 +862,18 @@ def _get_query_param_id(self, match, data):
         """
         if isinstance(data.get(match), int):
             return data[match]
-        else:
-            endpoint = CONVERT_TO_ID[match]
-            app = self._find_app(endpoint)
-            nb_app = getattr(self.nb, app)
-            nb_endpoint = getattr(nb_app, endpoint)
+        endpoint = CONVERT_TO_ID[match]
+        app = self._find_app(endpoint)
+        nb_app = getattr(self.nb, app)
+        nb_endpoint = getattr(nb_app, endpoint)
 
-            query_params = {QUERY_TYPES.get(match): data[match]}
-            result = self._nb_endpoint_get(nb_endpoint, query_params, match)
+        query_params = {QUERY_TYPES.get(match): data[match]}
+        result = self._nb_endpoint_get(nb_endpoint, query_params, match)
 
-            if result:
-                return result.id
-            else:
-                return data
+        if result:
+            return result.id
+        else:
+            return data
 
     def _build_query_params(
         self, parent, module_data, user_query_params=None, child=None
@@ -909,6 +915,16 @@ def _build_query_params(
 
                 if parent == "vlan_group" and match == "site":
                     query_dict.update({match: query_id})
+                elif (
+                    parent == "interface"
+                    and "device" in module_data
+                    and self._version_check_greater(
+                        self.version, "3.6", greater_or_equal=True
+                    )
+                ):
+                    query_dict.update(
+                        {"virtual_chassis_member_id": module_data["device"]}
+                    )
                 else:
                     query_dict.update({match + "_id": query_id})
             else:

tests/integration/netbox-deploy.py

@@ -343,6 +343,11 @@ def make_netbox_calls(endpoint, payload):
     devices[0]["location"] = created_rack_groups[0].id
     devices[1]["location"] = created_rack_groups[0].id
     devices[3]["location"] = created_rack_groups[0].id
+# TODO: Remove this logic and adjust payload from device_role -> role once Netbox 3.6 or greater is supported.
+if nb_version >= version.parse("3.6"):
+    for device in devices:
+        if "device_role" in device:
+            device["role"] = device.pop("device_role")
 
 created_devices = make_netbox_calls(nb.dcim.devices, devices)
 ### Device variables to be used later on

tests/integration/targets/inventory-v3.6/.gitignore

@@ -0,0 +1 @@
+runme_config

tests/integration/targets/inventory-v3.6/aliases

@@ -0,0 +1 @@
+# https://docs.ansible.com/ansible/devel/dev_guide/testing/sanity/integration-aliases.html

tests/integration/targets/inventory-v3.6/compare_inventory_json.py

@@ -0,0 +1,152 @@
+#!/usr/bin/env python
+
+# Inspired by community.aws collection script_inventory_ec2 test
+# https://github.com/ansible-collections/community.aws/blob/master/tests/integration/targets/script_inventory_ec2/inventory_diff.py
+
+from __future__ import absolute_import, division, print_function
+
+__metaclass__ = type
+
+import argparse
+import json
+import sys
+from operator import itemgetter
+
+from deepdiff import DeepDiff
+
+# NetBox includes "created" and "last_updated" times on objects. These end up in the interfaces objects that are included verbatim from the NetBox API.
+# "url" may be different if local tests use a different host/port
+# Remove these from files saved in git as test data
+KEYS_REMOVE = frozenset(["created", "last_updated", "url"])
+
+# Ignore these when performing diffs as they will be different for each test run
+# (Was previously keys specific to NetBox 2.6)
+KEYS_IGNORE = frozenset()
+
+# Rack Groups became hierarchical in NetBox 2.8. Don't bother comparing against test data in NetBox 2.7
+KEYS_IGNORE_27 = frozenset(
+    [
+        "rack_groups",  # host var
+        "rack_group_parent_rack_group",  # group, group_names_raw = False
+        "parent_rack_group",  # group, group_names_raw = True
+    ]
+)
+
+
+# Assume the object will not be recursive, as it originally came from JSON
+def remove_keys(obj, keys):
+    if isinstance(obj, dict):
+        keys_to_remove = keys.intersection(obj.keys())
+        for key in keys_to_remove:
+            del obj[key]
+
+        for key, value in obj.items():
+            remove_keys(value, keys)
+
+    elif isinstance(obj, list):
+        # Iterate over temporary copy, as we may remove items
+        for item in obj[:]:
+            if isinstance(item, str) and item in keys:
+                # List contains a string that we want to remove
+                # eg. a group name in list of groups
+                obj.remove(item)
+            remove_keys(item, keys)
+
+
+def sort_hostvar_arrays(obj):
+    meta = obj.get("_meta")
+    if not meta:
+        return
+
+    hostvars = meta.get("hostvars")
+    if not hostvars:
+        return
+
+    for _, host in hostvars.items():
+        if interfaces := host.get("interfaces"):
+            host["interfaces"] = sorted(interfaces, key=itemgetter("id"))
+
+        if services := host.get("services"):
+            host["services"] = sorted(services, key=itemgetter("id"))
+
+
+def read_json(filename):
+    with open(filename, "r", encoding="utf-8") as file:
+        return json.loads(file.read())
+
+
+def write_json(filename, data):
+    with open(filename, "w", encoding="utf-8") as file:
+        json.dump(data, file, indent=4)
+
+
+def main():
+    parser = argparse.ArgumentParser(description="Diff Ansible inventory JSON output")
+    parser.add_argument(
+        "filename_a",
+        metavar="ORIGINAL.json",
+        type=str,
+        help="Original json to test against",
+    )
+    parser.add_argument(
+        "filename_b",
+        metavar="NEW.json",
+        type=str,
+        help="Newly generated json to compare against original",
+    )
+    parser.add_argument(
+        "--write",
+        action="store_true",
+        help=(
+            "When comparing files, various keys are removed. "
+            "This option will not compare the files, and instead writes ORIGINAL.json to NEW.json after removing these keys. "
+            "This is used to clean the test json files before saving to the git repo. "
+            "For example, this removes dates. "
+        ),
+    )
+    parser.add_argument(
+        "--netbox-version",
+        metavar="VERSION",
+        type=str,
+        help=(
+            "Apply comparison specific to NetBox version. "
+            "For example, rack_groups arrays will only contain a single item in v2.7, so are ignored in the comparison."
+        ),
+    )
+
+    args = parser.parse_args()
+
+    data_a = read_json(args.filename_a)
+
+    if args.write:
+        # When writing test data, only remove "remove_keys" that will change on every git commit.
+        # This makes diffs more easily readable to ensure changes to test data look correct.
+        remove_keys(data_a, KEYS_REMOVE)
+        sort_hostvar_arrays(data_a)
+        write_json(args.filename_b, data_a)
+
+    else:
+        data_b = read_json(args.filename_b)
+
+        # Ignore keys that we don't want to diff, in addition to the ones removed that change on every commit
+        keys = KEYS_REMOVE.union(KEYS_IGNORE)
+        remove_keys(data_a, keys)
+        remove_keys(data_b, keys)
+
+        sort_hostvar_arrays(data_a)
+        sort_hostvar_arrays(data_b)
+
+        # Perform the diff
+        result = DeepDiff(data_a, data_b, ignore_order=True)
+
+        if result:
+            # Dictionary is not empty - print differences
+            print(json.dumps(result, sort_keys=True, indent=4))
+            sys.exit(1)
+        else:
+            # Success, no differences
+            sys.exit(0)
+
+
+if __name__ == "__main__":
+    main()