From b57ce54f07550dd03367355b613df9df2a75e7e3 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Thu, 2 Apr 2020 16:49:51 +1100 Subject: [PATCH 01/10] Add separate device_query_filters and vm_query_filters options #140 --- plugins/inventory/nb_inventory.py | 95 +++++++++++++++++++------------ 1 file changed, 60 insertions(+), 35 deletions(-) diff --git a/plugins/inventory/nb_inventory.py b/plugins/inventory/nb_inventory.py index 20723a736..f9b107a4f 100644 --- a/plugins/inventory/nb_inventory.py +++ b/plugins/inventory/nb_inventory.py @@ -79,7 +79,15 @@ type: boolean version_added: "0.2.0" query_filters: - description: List of parameters passed to the query string (Multiple values may be separated by commas) + description: List of parameters passed to the query string for both devices and VMs (Multiple values may be separated by commas) + type: list + default: [] + device_query_filters: + description: List of parameters passed to the query string for devices (Multiple values may be separated by commas) + type: list + default: [] + vm_query_filters: + description: List of parameters passed to the query string for VMs (Multiple values may be separated by commas) type: list default: [] timeout: @@ -136,6 +144,7 @@ from functools import partial from sys import version as python_version from threading import Thread +from typing import Iterable from itertools import chain from ansible.plugins.inventory import BaseInventoryPlugin, Constructable, Cacheable @@ -533,15 +542,15 @@ def refresh_lookups(self): for thread in thread_list: thread.join() - def validate_query_parameters(self, x, allowed_query_parameters): - if not (isinstance(x, dict) and len(x) == 1): + def validate_query_parameter(self, parameter, allowed_query_parameters): + if not (isinstance(parameter, dict) and len(parameter) == 1): self.display.warning( - "Warning query parameters %s not a dict with a single key." % x + "Warning query parameters %s not a dict with a single key." % parameter ) - return + return None - k = tuple(x.keys())[0] - v = tuple(x.values())[0] + k = tuple(parameter.keys())[0] + v = tuple(parameter.values())[0] if not (k in allowed_query_parameters or k.startswith("cf_")): msg = "Warning: %s not in %s or starting with cf (Custom field)" % ( @@ -549,50 +558,64 @@ def validate_query_parameters(self, x, allowed_query_parameters): allowed_query_parameters, ) self.display.warning(msg=msg) - return + return None return k, v + def filter_query_parameters(self, parameters, allowed_query_parameters): + return filter( + lambda parameter: parameter is not None, + # For each element of query_filters, test if it's allowed + map( + # Create a partial function with the device-specific list of query parameters + partial( + self.validate_query_parameter, + allowed_query_parameters=allowed_query_parameters, + ), + parameters + ) + ) + def refresh_url(self): - dev_query_parameters = [("limit", 0)] + device_query_parameters = [("limit", 0)] vm_query_parameters = [("limit", 0)] device_url = self.api_endpoint + "/api/dcim/devices/?" vm_url = self.api_endpoint + "/api/virtualization/virtual-machines/?" - if self.query_filters: - dev_query_parameters.extend( - filter( - lambda x: x, - map( - partial( - self.validate_query_parameters, - allowed_query_parameters=ALLOWED_DEVICE_QUERY_PARAMETERS, - ), - self.query_filters, - ), - ) - ) + + # Add query_filtes to both devices and vms query, if they're valid + if isinstance(self.query_filters, Iterable): + device_query_parameters.extend( + self.filter_query_parameters(self.query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS)) + vm_query_parameters.extend( - filter( - lambda x: x, - map( - partial( - self.validate_query_parameters, - allowed_query_parameters=ALLOWED_VM_QUERY_PARAMETERS, - ), - self.query_filters, - ), - ) - ) - if len(dev_query_parameters) <= 1: + self.filter_query_parameters(self.query_filters, ALLOWED_VM_QUERY_PARAMETERS)) + + if isinstance(self.device_query_filters, Iterable): + device_query_parameters.extend( + self.filter_query_parameters(self.device_query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS)) + + if isinstance(self.vm_query_filters, Iterable): + vm_query_parameters.extend( + self.filter_query_parameters(self.vm_query_filters, ALLOWED_VM_QUERY_PARAMETERS)) + + # When query_filters is Iterable, and is not empty: + # - If none of the filters are valid for devices, do not fetch any devices + # - If none of the filters are valid for VMs, do not fetch any VMs + # If either device_query_filters or vm_query_filters are set, + # device_query_parameters and vm_query_parameters will have > 1 element so will continue to be requested + if self.query_filters and isinstance(self.query_filters, Iterable): + if len(device_query_parameters) <= 1: device_url = None if len(vm_query_parameters) <= 1: vm_url = None + # Append the parameters to the URLs if device_url: - device_url = device_url + urlencode(dev_query_parameters) + device_url = device_url + urlencode(device_query_parameters) if vm_url: vm_url = vm_url + urlencode(vm_query_parameters) + # Exclude config_context if not required if not self.config_context: if device_url: device_url = device_url + "&exclude=config_context" @@ -706,4 +729,6 @@ def parse(self, inventory, loader, path, cache=True): self.group_by = self.get_option("group_by") self.group_names_raw = self.get_option("group_names_raw") self.query_filters = self.get_option("query_filters") + self.device_query_filters = self.get_option("device_query_filters") + self.vm_query_filters = self.get_option("vm_query_filters") self.main() From 0917d36aec0659a5b29e1cb04b811d42a352b583 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Thu, 2 Apr 2020 22:33:46 +1100 Subject: [PATCH 02/10] Start creating unit tests for inventory plugin #56 I've specifically started with tests for #140 new query_filters options and refresh_url --- tests/test_data.py | 17 ++ tests/unit/inventory/__init__.py | 0 .../filter_query_parameters/data.json | 35 ++++ .../inventory/test_data/refresh_url/data.json | 167 ++++++++++++++++++ .../validate_query_parameter/data.json | 26 +++ tests/unit/inventory/test_nb_inventory.py | 85 +++++++++ .../module_utils/test_netbox_base_class.py | 34 ++-- 7 files changed, 345 insertions(+), 19 deletions(-) create mode 100644 tests/test_data.py create mode 100644 tests/unit/inventory/__init__.py create mode 100644 tests/unit/inventory/test_data/filter_query_parameters/data.json create mode 100644 tests/unit/inventory/test_data/refresh_url/data.json create mode 100644 tests/unit/inventory/test_data/validate_query_parameter/data.json create mode 100644 tests/unit/inventory/test_nb_inventory.py diff --git a/tests/test_data.py b/tests/test_data.py new file mode 100644 index 000000000..b7cecabc5 --- /dev/null +++ b/tests/test_data.py @@ -0,0 +1,17 @@ +# -*- coding: utf-8 -*- +# Copyright: (c) 2019, Bruno Inec (@sweenu) +# Copyright: (c) 2019, Mikhail Yohman (@FragmentedPacket) +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +import json + +# Load test data from a json file, for a pytest parametrize +def load_test_data(path, test_path): + with open(f"{path}/test_data/{test_path}/data.json", "r") as f: + data = json.loads(f.read()) + tests = [] + for test in data: + tuple_data = tuple(test.values()) + tests.append(tuple_data) + return tests + diff --git a/tests/unit/inventory/__init__.py b/tests/unit/inventory/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/inventory/test_data/filter_query_parameters/data.json b/tests/unit/inventory/test_data/filter_query_parameters/data.json new file mode 100644 index 000000000..3a79bcc2a --- /dev/null +++ b/tests/unit/inventory/test_data/filter_query_parameters/data.json @@ -0,0 +1,35 @@ +[ + { + "parameters": [ + {"a": "value1"}, + {"b": "value2"} + ], + "expected": [ + {"a": "value1"}, + {"b": "value2"} + ] + }, + { + "parameters": [ + {"z": "value1"}, + {"b": "value2"} + ], + "expected": [ + {"b": "value2"} + ] + }, + { + "parameters": [ + {"x": "value1"}, + {"y": "value2"} + ], + "expected": [ + ] + }, + { + "parameters": { + }, + "expected": [ + ] + } +] diff --git a/tests/unit/inventory/test_data/refresh_url/data.json b/tests/unit/inventory/test_data/refresh_url/data.json new file mode 100644 index 000000000..8be2290b2 --- /dev/null +++ b/tests/unit/inventory/test_data/refresh_url/data.json @@ -0,0 +1,167 @@ +[ + { + "options": { + "query_filters": [ + ], + "device_query_filters": { + }, + "vm_query_filters": { + }, + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0" + ] + }, + { + "options": { + "query_filters": 12345, + "device_query_filters": 6543, + "vm_query_filters": null, + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0" + ] + }, + { + "options": { + "query_filters": [ + ], + "device_query_filters": { + }, + "vm_query_filters": { + }, + "config_context": false + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&exclude=config_context", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&exclude=config_context" + ] + }, + { + "options": { + "query_filters": [ + {"name": "name value"}, + {"region": "region value"} + ], + "device_query_filters": { + }, + "vm_query_filters": { + }, + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&name=name+value®ion=region+value", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&name=name+value®ion=region+value" + ] + }, + { + "options": { + "query_filters": [ + {"name": "name value"} + ], + "device_query_filters": [ + {"region": "device"} + ], + "vm_query_filters": [ + {"region": "vm"} + ], + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&name=name+value®ion=device", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&name=name+value®ion=vm" + ] + }, + { + "options": { + "query_filters": [ + {"name": "name value"}, + {"invalid query filter": "nope"} + ], + "device_query_filters": [ + {"has_primary_ip": "true"}, + {"invalid device filter": "nope"} + ], + "vm_query_filters": [ + {"disk": 42}, + {"invalid vm filter": "nope"} + ], + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&name=name+value&has_primary_ip=true", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&name=name+value&disk=42" + ] + }, + { + "options": { + "query_filters": [ + {"disk": "3"} + ], + "device_query_filters": [ + ], + "vm_query_filters": [ + ], + "config_context": true + }, + "expected": [ + null, + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&disk=3" + ] + }, + { + "options": { + "query_filters": [ + {"has_primary_ip": "true"} + ], + "device_query_filters": [ + ], + "vm_query_filters": [ + ], + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&has_primary_ip=true", + null + ] + }, + { + "options": { + "query_filters": [ + {"disk": "3"} + ], + "device_query_filters": [ + {"name": "name value"} + ], + "vm_query_filters": [ + ], + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&name=name+value", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&disk=3" + ] + }, + { + "options": { + "query_filters": [ + {"has_primary_ip": "true"} + ], + "device_query_filters": [ + ], + "vm_query_filters": [ + {"name": "name value"} + ], + "config_context": true + }, + "expected": [ + "https://netbox.test.endpoint:1234/api/dcim/devices/?limit=0&has_primary_ip=true", + "https://netbox.test.endpoint:1234/api/virtualization/virtual-machines/?limit=0&name=name+value" + ] + } + +] diff --git a/tests/unit/inventory/test_data/validate_query_parameter/data.json b/tests/unit/inventory/test_data/validate_query_parameter/data.json new file mode 100644 index 000000000..6e95d9904 --- /dev/null +++ b/tests/unit/inventory/test_data/validate_query_parameter/data.json @@ -0,0 +1,26 @@ +[ + { + "parameter": "a", + "expected": true + }, + { + "parameter": "b", + "expected": true + }, + { + "parameter": "c", + "expected": true + }, + { + "parameter": "x", + "expected": false + }, + { + "parameter": "y", + "expected": false + }, + { + "parameter": "z", + "expected": false + } +] diff --git a/tests/unit/inventory/test_nb_inventory.py b/tests/unit/inventory/test_nb_inventory.py new file mode 100644 index 000000000..4b90cbc7d --- /dev/null +++ b/tests/unit/inventory/test_nb_inventory.py @@ -0,0 +1,85 @@ +# -*- coding: utf-8 -*- +# Copyright: (c) 2020, Hillsong, Douglas Heriot (@DouglasHeriot) +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +import pytest +import os +from functools import partial +from unittest.mock import patch, MagicMock, Mock + +try: + from ansible_collections.netbox.netbox.plugins.inventory.nb_inventory import ( + InventoryModule, + ) + from ansible_collections.netbox.netbox.tests.test_data import load_test_data + +except ImportError: + import sys + + # Not installed as a collection + # Try importing relative to root directory of this ansible_modules project + + sys.path.append("plugins/inventory") + sys.path.append("tests") + from nb_inventory import InventoryModule + from test_data import load_test_data + +load_relative_test_data = partial(load_test_data, os.path.dirname(os.path.abspath(__file__))) + + +@pytest.fixture +def inventory_fixture(): + # TODO: Mock _fetch_information() to return static HTTP responses + inventory = InventoryModule() + inventory.api_endpoint = "https://netbox.test.endpoint:1234" + return inventory + + +@pytest.fixture +def allowed_query_parameters_fixture(): + return ["a", "b", "c"] + + +@pytest.mark.parametrize("parameter, expected", load_relative_test_data("validate_query_parameter")) +def test_validate_query_parameter( + inventory_fixture, + allowed_query_parameters_fixture, + parameter, expected): + + value = "some value, doesn't matter" + result = inventory_fixture.validate_query_parameter({parameter: value}, allowed_query_parameters_fixture) + assert (result == (parameter, value)) == expected + + +@pytest.mark.parametrize("parameters, expected", load_relative_test_data("filter_query_parameters")) +def test_filter_query_parameters( + inventory_fixture, + allowed_query_parameters_fixture, + parameters, expected): + + result = inventory_fixture.filter_query_parameters(parameters, allowed_query_parameters_fixture) + + # Result is iterators of tuples + # expected from json file is an array of dicts + + # Convert result iterator to list so we can get the length, and iterate over with an index + result_list = list(result) + + assert len(result_list) == len(expected) + + for i, parameter in enumerate(result_list): + assert parameter[0] == list(expected[i].keys())[0] + assert parameter[1] == list(expected[i].values())[0] + + +@pytest.mark.parametrize("options, expected", load_relative_test_data("refresh_url")) +def test_refresh_url(inventory_fixture, options, expected): + + inventory_fixture.query_filters = options["query_filters"] + inventory_fixture.device_query_filters = options["device_query_filters"] + inventory_fixture.vm_query_filters = options["vm_query_filters"] + inventory_fixture.config_context = options["config_context"] + + result = inventory_fixture.refresh_url() + + assert result == tuple(expected) diff --git a/tests/unit/module_utils/test_netbox_base_class.py b/tests/unit/module_utils/test_netbox_base_class.py index c3201041d..831cbd01e 100644 --- a/tests/unit/module_utils/test_netbox_base_class.py +++ b/tests/unit/module_utils/test_netbox_base_class.py @@ -4,8 +4,8 @@ # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) import pytest -import json import os +from functools import partial from unittest.mock import patch, MagicMock, Mock from ansible.module_utils.basic import AnsibleModule @@ -16,17 +16,24 @@ from ansible_collections.netbox.netbox.plugins.module_utils.netbox_dcim import ( NB_DEVICES, ) + from ansible_collections.netbox.netbox.tests.test_data import load_test_data MOCKER_PATCH_PATH = "ansible_collections.netbox.netbox.plugins.module_utils.netbox_utils.NetboxModule" except ImportError: import sys + # Not installed as a collection + # Try importing relative to root directory of this ansible_modules project + sys.path.append("plugins/module_utils") + sys.path.append("tests") from netbox_utils import NetboxModule from netbox_dcim import NB_DEVICES + from test_data import load_test_data MOCKER_PATCH_PATH = "netbox_utils.NetboxModule" +load_relative_test_data = partial(load_test_data, os.path.dirname(os.path.abspath(__file__))) @pytest.fixture def fixture_arg_spec(): @@ -148,39 +155,28 @@ def test_init(mock_netbox_module, find_ids_return): assert mock_netbox_module.data == find_ids_return -def load_test_data(test_path): - path = os.path.dirname(os.path.abspath(__file__)) - with open(f"{path}/test_data/{test_path}/data.json", "r") as f: - data = json.loads(f.read()) - tests = [] - for test in data: - tuple_data = tuple(test.values()) - tests.append(tuple_data) - return tests - - -@pytest.mark.parametrize("before, after", load_test_data("normalize_data")) +@pytest.mark.parametrize("before, after", load_relative_test_data("normalize_data")) def test_normalize_data_returns_correct_data(mock_netbox_module, before, after): norm_data = mock_netbox_module._normalize_data(before) assert norm_data == after -@pytest.mark.parametrize("data, expected", load_test_data("arg_spec_default")) +@pytest.mark.parametrize("data, expected", load_relative_test_data("arg_spec_default")) def test_remove_arg_spec_defaults(mock_netbox_module, data, expected): new_data = mock_netbox_module._remove_arg_spec_default(data) assert new_data == expected -@pytest.mark.parametrize("non_slug, expected", load_test_data("slug")) +@pytest.mark.parametrize("non_slug, expected", load_relative_test_data("slug")) def test_to_slug_returns_valid_slug(mock_netbox_module, non_slug, expected): got_slug = mock_netbox_module._to_slug(non_slug) assert got_slug == expected -@pytest.mark.parametrize("endpoint, app", load_test_data("find_app")) +@pytest.mark.parametrize("endpoint, app", load_relative_test_data("find_app")) def test_find_app_returns_valid_app(mock_netbox_module, endpoint, app): assert app == mock_netbox_module._find_app(endpoint), "app: %s, endpoint: %s" % ( app, @@ -188,7 +184,7 @@ def test_find_app_returns_valid_app(mock_netbox_module, endpoint, app): ) -@pytest.mark.parametrize("endpoint, data, expected", load_test_data("choices_id")) +@pytest.mark.parametrize("endpoint, data, expected", load_relative_test_data("choices_id")) def test_change_choices_id(mocker, mock_netbox_module, endpoint, data, expected): fetch_choice_value = mocker.patch( "%s%s" % (MOCKER_PATCH_PATH, "._fetch_choice_value") @@ -199,7 +195,7 @@ def test_change_choices_id(mocker, mock_netbox_module, endpoint, data, expected) @pytest.mark.parametrize( - "parent, module_data, expected", load_test_data("build_query_params_no_child") + "parent, module_data, expected", load_relative_test_data("build_query_params_no_child") ) def test_build_query_params_no_child( mock_netbox_module, mocker, parent, module_data, expected @@ -213,7 +209,7 @@ def test_build_query_params_no_child( @pytest.mark.parametrize( - "parent, module_data, child, expected", load_test_data("build_query_params_child") + "parent, module_data, child, expected", load_relative_test_data("build_query_params_child") ) def test_build_query_params_child( mock_netbox_module, mocker, parent, module_data, child, expected From 9c61f67d72b78b7ce94dffd91e5f14d2248b5494 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Thu, 2 Apr 2020 23:29:40 +1100 Subject: [PATCH 03/10] Report code coverage of unit tests #56 --- .travis.yml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index f592908a3..fde9d9aae 100644 --- a/.travis.yml +++ b/.travis.yml @@ -28,7 +28,8 @@ jobs: - docker-compose up -d - cd .. - pip install -U pip - - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 + # coverage 5.0 is not compatible with ansible-test https://github.com/ansible/ansible/issues/65907 + - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 "coverage<5" - pip install pynetbox cryptography codecov jmespath ansible # Latest release of Netbox and Ansible @@ -44,7 +45,7 @@ jobs: - docker-compose up -d - cd .. - pip install -U pip - - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 + - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 "coverage<5" - pip install pynetbox cryptography codecov jmespath ansible # Latest development versions of Netbox and Ansible, newest Python @@ -62,7 +63,7 @@ jobs: - docker-compose up -d - cd .. - pip install -U pip - - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 + - pip install pytest==4.6.5 pytest-mock pytest-xdist jinja2 PyYAML black==19.10b0 "coverage<5" - pip install pynetbox cryptography jmespath - git clone https://github.com/ansible/ansible.git - cd ansible @@ -82,7 +83,7 @@ before_script: script: # Perform unit tests on collection from within the installed directory, not the source directory # Required for imports of other collections (ie. ansible.netcommon) to work correctly - - (cd /home/travis/.ansible/collections/ansible_collections/$COLLECTION_NAMESPACE/$COLLECTION_NAME && ansible-test units --python $PYTHON_VER -v) + - (cd /home/travis/.ansible/collections/ansible_collections/$COLLECTION_NAMESPACE/$COLLECTION_NAME && ansible-test units --coverage --python $PYTHON_VER -v) # Check python syntax - black . --check --diff @@ -98,6 +99,9 @@ script: - ansible-playbook tests/integration/$VERSION/main.yml -vvvv - ansible-inventory -i tests/integration/test-inventory.yml --list + # Report code coverage + - (cd /home/travis/.ansible/collections/ansible_collections/$COLLECTION_NAMESPACE/$COLLECTION_NAME && ansible-test coverage report) + deploy: provider: script skip_cleanup: true From ac5c783bdef30278acebb45f5c9b154654779496 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Thu, 2 Apr 2020 23:30:08 +1100 Subject: [PATCH 04/10] Remove unused test fixture #56 --- tests/unit/module_utils/test_netbox_base_class.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/unit/module_utils/test_netbox_base_class.py b/tests/unit/module_utils/test_netbox_base_class.py index 831cbd01e..913c650ea 100644 --- a/tests/unit/module_utils/test_netbox_base_class.py +++ b/tests/unit/module_utils/test_netbox_base_class.py @@ -53,13 +53,6 @@ def fixture_arg_spec(): } -@pytest.fixture -def choices_data(choice): - with open(f"{choice}.json", "r") as f: - choice_data = f.read() - return choice_data - - @pytest.fixture def normalized_data(): return { From c2bd50481742124654deab30c77d0518b380c074 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Thu, 2 Apr 2020 23:30:46 +1100 Subject: [PATCH 05/10] Raise exceptions if inventory options are not valid #140 --- plugins/inventory/nb_inventory.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/plugins/inventory/nb_inventory.py b/plugins/inventory/nb_inventory.py index f9b107a4f..cd39b1cb5 100644 --- a/plugins/inventory/nb_inventory.py +++ b/plugins/inventory/nb_inventory.py @@ -731,4 +731,15 @@ def parse(self, inventory, loader, path, cache=True): self.query_filters = self.get_option("query_filters") self.device_query_filters = self.get_option("device_query_filters") self.vm_query_filters = self.get_option("vm_query_filters") + + # Basic type checking of options + if self.query_filters != None and not isinstance(self.query_filters, Iterable): + raise AnsibleError("query_filters is set, but is not a list of dictionaries", obj=self.query_filters, show_content=True) + + if self.device_query_filters != None and not isinstance(self.device_query_filters, Iterable): + raise AnsibleError("device_query_filters is set, but is not a list of dictionaries", obj=self.device_query_filters, show_content=True) + + if self.vm_query_filters != None and not isinstance(self.vm_query_filters, Iterable): + raise AnsibleError("vm_query_filters is set, but is not a list of dictionaries", obj=self.vm_query_filters, show_content=True) + self.main() From d2743bce7d9d61223dcc69451aa28930ee847f84 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Fri, 3 Apr 2020 00:38:21 +1100 Subject: [PATCH 06/10] Update ALLOWED_DEVICE_QUERY_PARAMETERS and ALLOWED_VM_QUERY_PARAMETERS (#140) Wrote a quite little script to fetch these once-off when this list needs updating. In future, this list could be fetched at runtime by querying the API /api/docs/?format=openapi --- plugins/inventory/nb_inventory.py | 175 +++++++++++++++++++++- scripts/get_inventory_query_parameters.py | 38 +++++ 2 files changed, 208 insertions(+), 5 deletions(-) create mode 100755 scripts/get_inventory_query_parameters.py diff --git a/plugins/inventory/nb_inventory.py b/plugins/inventory/nb_inventory.py index cd39b1cb5..54aba2298 100644 --- a/plugins/inventory/nb_inventory.py +++ b/plugins/inventory/nb_inventory.py @@ -157,65 +157,230 @@ ip_interface, ) +# List of parameters fetched from /api/docs/?format=openapi +# Use scripts/get_inventory_query_parameters.py to update this + ALLOWED_DEVICE_QUERY_PARAMETERS = ( "asset_tag", + "asset_tag__ic", + "asset_tag__ie", + "asset_tag__iew", + "asset_tag__isw", + "asset_tag__n", + "asset_tag__nic", + "asset_tag__nie", + "asset_tag__niew", + "asset_tag__nisw", "cluster_id", + "cluster_id__n", + "console_ports", + "console_server_ports", + "created", + "created__gte", + "created__lte", + "device_bays", "device_type_id", + "device_type_id__n", + "face", + "face__n", "has_primary_ip", - "is_console_server", + "id", + "id__gt", + "id__gte", + "id__in", + "id__lt", + "id__lte", + "id__n", + "interfaces", "is_full_depth", - "is_network_device", - "is_pdu", + "last_updated", + "last_updated__gte", + "last_updated__lte", + "limit", + "local_context_data", "mac_address", + "mac_address__ic", + "mac_address__ie", + "mac_address__iew", + "mac_address__isw", + "mac_address__n", + "mac_address__nic", + "mac_address__nie", + "mac_address__niew", + "mac_address__nisw", "manufacturer", + "manufacturer__n", "manufacturer_id", + "manufacturer_id__n", "model", + "model__n", "name", + "name__ic", + "name__ie", + "name__iew", + "name__isw", + "name__n", + "name__nic", + "name__nie", + "name__niew", + "name__nisw", + "offset", + "pass_through_ports", "platform", + "platform__n", "platform_id", + "platform_id__n", "position", + "position__gt", + "position__gte", + "position__lt", + "position__lte", + "position__n", + "power_outlets", + "power_ports", + "q", "rack_group_id", + "rack_group_id__n", "rack_id", + "rack_id__n", "region", + "region__n", "region_id", + "region_id__n", "role", + "role__n", "role_id", + "role_id__n", "serial", "site", + "site__n", "site_id", + "site_id__n", "status", + "status__n", "tag", + "tag__n", "tenant", + "tenant__n", + "tenant_group", + "tenant_group__n", + "tenant_group_id", + "tenant_group_id__n", "tenant_id", + "tenant_id__n", + "vc_position", + "vc_position__gt", + "vc_position__gte", + "vc_position__lt", + "vc_position__lte", + "vc_position__n", + "vc_priority", + "vc_priority__gt", + "vc_priority__gte", + "vc_priority__lt", + "vc_priority__lte", + "vc_priority__n", "virtual_chassis_id", + "virtual_chassis_id__n", + "virtual_chassis_member", ) ALLOWED_VM_QUERY_PARAMETERS = ( "cluster", - "cluster_id", + "cluster__n", "cluster_group", + "cluster_group__n", "cluster_group_id", + "cluster_group_id__n", + "cluster_id", + "cluster_id__n", "cluster_type", + "cluster_type__n", "cluster_type_id", + "cluster_type_id__n", + "created", + "created__gte", + "created__lte", "disk", + "disk__gt", + "disk__gte", + "disk__lt", + "disk__lte", + "disk__n", + "id", + "id__gt", + "id__gte", + "id__in", + "id__lt", + "id__lte", + "id__n", + "last_updated", + "last_updated__gte", + "last_updated__lte", + "limit", + "local_context_data", "mac_address", + "mac_address__ic", + "mac_address__ie", + "mac_address__iew", + "mac_address__isw", + "mac_address__n", + "mac_address__nic", + "mac_address__nie", + "mac_address__niew", + "mac_address__nisw", "memory", + "memory__gt", + "memory__gte", + "memory__lt", + "memory__lte", + "memory__n", "name", + "name__ic", + "name__ie", + "name__iew", + "name__isw", + "name__n", + "name__nic", + "name__nie", + "name__niew", + "name__nisw", + "offset", "platform", + "platform__n", "platform_id", + "platform_id__n", + "q", "region", + "region__n", "region_id", + "region_id__n", "role", + "role__n", "role_id", + "role_id__n", "site", + "site__n", "site_id", + "site_id__n", "status", + "status__n", "tag", + "tag__n", "tenant", - "tenant_id", + "tenant__n", "tenant_group", + "tenant_group__n", "tenant_group_id", + "tenant_group_id__n", + "tenant_id", + "tenant_id__n", "vcpus", + "vcpus__gt", + "vcpus__gte", + "vcpus__lt", + "vcpus__lte", + "vcpus__n", ) diff --git a/scripts/get_inventory_query_parameters.py b/scripts/get_inventory_query_parameters.py new file mode 100755 index 000000000..8ef297341 --- /dev/null +++ b/scripts/get_inventory_query_parameters.py @@ -0,0 +1,38 @@ +#!/usr/bin/env python3 + +# Usage: ./get_inventory_query_filters.py https://netbox/api/docs/?format=openapi + +import sys +import json +import urllib.request + + +def get_parameters(data, path): + output = list() + + parameters = data["paths"][path]["get"]["parameters"] + for p in parameters: + output.append(p["name"]) + + # Sort, so git diffs are nice and consistent when parameters are updated + output.sort() + return output + + +url = sys.argv[1] + +print("Getting from %s" % url, file=sys.stderr) + +response = urllib.request.urlopen(url) +data = json.load(response) + +print("ALLOWED_DEVICE_QUERY_PARAMETERS = (") +for p in get_parameters(data, "/dcim/devices/"): + print(" \"%s\"," % p) +print(")") +print() +print("ALLOWED_VM_QUERY_PARAMETERS = (") +for p in get_parameters(data, "/virtualization/virtual-machines/"): + print(" \"%s\"," % p) +print(")") +print() From fafd265896eea571e7afdc0b400902cddc613278 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Fri, 3 Apr 2020 00:43:13 +1100 Subject: [PATCH 07/10] Apply black python formatting --- plugins/inventory/nb_inventory.py | 50 +++++++++++++++---- scripts/get_inventory_query_parameters.py | 18 +++---- tests/test_data.py | 1 - tests/unit/inventory/test_nb_inventory.py | 30 +++++++---- .../module_utils/test_netbox_base_class.py | 15 ++++-- 5 files changed, 78 insertions(+), 36 deletions(-) diff --git a/plugins/inventory/nb_inventory.py b/plugins/inventory/nb_inventory.py index 54aba2298..fffc8f4cb 100644 --- a/plugins/inventory/nb_inventory.py +++ b/plugins/inventory/nb_inventory.py @@ -736,8 +736,8 @@ def filter_query_parameters(self, parameters, allowed_query_parameters): self.validate_query_parameter, allowed_query_parameters=allowed_query_parameters, ), - parameters - ) + parameters, + ), ) def refresh_url(self): @@ -749,18 +749,30 @@ def refresh_url(self): # Add query_filtes to both devices and vms query, if they're valid if isinstance(self.query_filters, Iterable): device_query_parameters.extend( - self.filter_query_parameters(self.query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS)) + self.filter_query_parameters( + self.query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS + ) + ) vm_query_parameters.extend( - self.filter_query_parameters(self.query_filters, ALLOWED_VM_QUERY_PARAMETERS)) + self.filter_query_parameters( + self.query_filters, ALLOWED_VM_QUERY_PARAMETERS + ) + ) if isinstance(self.device_query_filters, Iterable): device_query_parameters.extend( - self.filter_query_parameters(self.device_query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS)) + self.filter_query_parameters( + self.device_query_filters, ALLOWED_DEVICE_QUERY_PARAMETERS + ) + ) if isinstance(self.vm_query_filters, Iterable): vm_query_parameters.extend( - self.filter_query_parameters(self.vm_query_filters, ALLOWED_VM_QUERY_PARAMETERS)) + self.filter_query_parameters( + self.vm_query_filters, ALLOWED_VM_QUERY_PARAMETERS + ) + ) # When query_filters is Iterable, and is not empty: # - If none of the filters are valid for devices, do not fetch any devices @@ -899,12 +911,28 @@ def parse(self, inventory, loader, path, cache=True): # Basic type checking of options if self.query_filters != None and not isinstance(self.query_filters, Iterable): - raise AnsibleError("query_filters is set, but is not a list of dictionaries", obj=self.query_filters, show_content=True) + raise AnsibleError( + "query_filters is set, but is not a list of dictionaries", + obj=self.query_filters, + show_content=True, + ) - if self.device_query_filters != None and not isinstance(self.device_query_filters, Iterable): - raise AnsibleError("device_query_filters is set, but is not a list of dictionaries", obj=self.device_query_filters, show_content=True) + if self.device_query_filters != None and not isinstance( + self.device_query_filters, Iterable + ): + raise AnsibleError( + "device_query_filters is set, but is not a list of dictionaries", + obj=self.device_query_filters, + show_content=True, + ) - if self.vm_query_filters != None and not isinstance(self.vm_query_filters, Iterable): - raise AnsibleError("vm_query_filters is set, but is not a list of dictionaries", obj=self.vm_query_filters, show_content=True) + if self.vm_query_filters != None and not isinstance( + self.vm_query_filters, Iterable + ): + raise AnsibleError( + "vm_query_filters is set, but is not a list of dictionaries", + obj=self.vm_query_filters, + show_content=True, + ) self.main() diff --git a/scripts/get_inventory_query_parameters.py b/scripts/get_inventory_query_parameters.py index 8ef297341..d54421c82 100755 --- a/scripts/get_inventory_query_parameters.py +++ b/scripts/get_inventory_query_parameters.py @@ -8,15 +8,15 @@ def get_parameters(data, path): - output = list() + output = list() - parameters = data["paths"][path]["get"]["parameters"] - for p in parameters: - output.append(p["name"]) + parameters = data["paths"][path]["get"]["parameters"] + for p in parameters: + output.append(p["name"]) - # Sort, so git diffs are nice and consistent when parameters are updated - output.sort() - return output + # Sort, so git diffs are nice and consistent when parameters are updated + output.sort() + return output url = sys.argv[1] @@ -28,11 +28,11 @@ def get_parameters(data, path): print("ALLOWED_DEVICE_QUERY_PARAMETERS = (") for p in get_parameters(data, "/dcim/devices/"): - print(" \"%s\"," % p) + print(' "%s",' % p) print(")") print() print("ALLOWED_VM_QUERY_PARAMETERS = (") for p in get_parameters(data, "/virtualization/virtual-machines/"): - print(" \"%s\"," % p) + print(' "%s",' % p) print(")") print() diff --git a/tests/test_data.py b/tests/test_data.py index b7cecabc5..4d082e217 100644 --- a/tests/test_data.py +++ b/tests/test_data.py @@ -14,4 +14,3 @@ def load_test_data(path, test_path): tuple_data = tuple(test.values()) tests.append(tuple_data) return tests - diff --git a/tests/unit/inventory/test_nb_inventory.py b/tests/unit/inventory/test_nb_inventory.py index 4b90cbc7d..66ccbd953 100644 --- a/tests/unit/inventory/test_nb_inventory.py +++ b/tests/unit/inventory/test_nb_inventory.py @@ -24,7 +24,9 @@ from nb_inventory import InventoryModule from test_data import load_test_data -load_relative_test_data = partial(load_test_data, os.path.dirname(os.path.abspath(__file__))) +load_relative_test_data = partial( + load_test_data, os.path.dirname(os.path.abspath(__file__)) +) @pytest.fixture @@ -40,24 +42,30 @@ def allowed_query_parameters_fixture(): return ["a", "b", "c"] -@pytest.mark.parametrize("parameter, expected", load_relative_test_data("validate_query_parameter")) +@pytest.mark.parametrize( + "parameter, expected", load_relative_test_data("validate_query_parameter") +) def test_validate_query_parameter( - inventory_fixture, - allowed_query_parameters_fixture, - parameter, expected): + inventory_fixture, allowed_query_parameters_fixture, parameter, expected +): value = "some value, doesn't matter" - result = inventory_fixture.validate_query_parameter({parameter: value}, allowed_query_parameters_fixture) + result = inventory_fixture.validate_query_parameter( + {parameter: value}, allowed_query_parameters_fixture + ) assert (result == (parameter, value)) == expected -@pytest.mark.parametrize("parameters, expected", load_relative_test_data("filter_query_parameters")) +@pytest.mark.parametrize( + "parameters, expected", load_relative_test_data("filter_query_parameters") +) def test_filter_query_parameters( - inventory_fixture, - allowed_query_parameters_fixture, - parameters, expected): + inventory_fixture, allowed_query_parameters_fixture, parameters, expected +): - result = inventory_fixture.filter_query_parameters(parameters, allowed_query_parameters_fixture) + result = inventory_fixture.filter_query_parameters( + parameters, allowed_query_parameters_fixture + ) # Result is iterators of tuples # expected from json file is an array of dicts diff --git a/tests/unit/module_utils/test_netbox_base_class.py b/tests/unit/module_utils/test_netbox_base_class.py index 913c650ea..3408e06d3 100644 --- a/tests/unit/module_utils/test_netbox_base_class.py +++ b/tests/unit/module_utils/test_netbox_base_class.py @@ -33,7 +33,10 @@ MOCKER_PATCH_PATH = "netbox_utils.NetboxModule" -load_relative_test_data = partial(load_test_data, os.path.dirname(os.path.abspath(__file__))) +load_relative_test_data = partial( + load_test_data, os.path.dirname(os.path.abspath(__file__)) +) + @pytest.fixture def fixture_arg_spec(): @@ -177,7 +180,9 @@ def test_find_app_returns_valid_app(mock_netbox_module, endpoint, app): ) -@pytest.mark.parametrize("endpoint, data, expected", load_relative_test_data("choices_id")) +@pytest.mark.parametrize( + "endpoint, data, expected", load_relative_test_data("choices_id") +) def test_change_choices_id(mocker, mock_netbox_module, endpoint, data, expected): fetch_choice_value = mocker.patch( "%s%s" % (MOCKER_PATCH_PATH, "._fetch_choice_value") @@ -188,7 +193,8 @@ def test_change_choices_id(mocker, mock_netbox_module, endpoint, data, expected) @pytest.mark.parametrize( - "parent, module_data, expected", load_relative_test_data("build_query_params_no_child") + "parent, module_data, expected", + load_relative_test_data("build_query_params_no_child"), ) def test_build_query_params_no_child( mock_netbox_module, mocker, parent, module_data, expected @@ -202,7 +208,8 @@ def test_build_query_params_no_child( @pytest.mark.parametrize( - "parent, module_data, child, expected", load_relative_test_data("build_query_params_child") + "parent, module_data, child, expected", + load_relative_test_data("build_query_params_child"), ) def test_build_query_params_child( mock_netbox_module, mocker, parent, module_data, child, expected From 88d62d0bfa6caaad5fe5edf9b729f6b339e0d087 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Tue, 14 Apr 2020 12:44:22 +1000 Subject: [PATCH 08/10] Inventory: Test validate_query_parameter against actualy parameters in nb_inventory (#140) --- .../test_data/filter_query_parameters/data.json | 12 ++++++------ .../test_data/validate_query_parameter/data.json | 6 +++--- tests/unit/inventory/test_nb_inventory.py | 16 ++++++++-------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/unit/inventory/test_data/filter_query_parameters/data.json b/tests/unit/inventory/test_data/filter_query_parameters/data.json index 3a79bcc2a..a9c224baa 100644 --- a/tests/unit/inventory/test_data/filter_query_parameters/data.json +++ b/tests/unit/inventory/test_data/filter_query_parameters/data.json @@ -1,21 +1,21 @@ [ { "parameters": [ - {"a": "value1"}, - {"b": "value2"} + {"name": "value1"}, + {"platform": "value2"} ], "expected": [ - {"a": "value1"}, - {"b": "value2"} + {"name": "value1"}, + {"platform": "value2"} ] }, { "parameters": [ {"z": "value1"}, - {"b": "value2"} + {"platform": "value2"} ], "expected": [ - {"b": "value2"} + {"platform": "value2"} ] }, { diff --git a/tests/unit/inventory/test_data/validate_query_parameter/data.json b/tests/unit/inventory/test_data/validate_query_parameter/data.json index 6e95d9904..a7fca74cf 100644 --- a/tests/unit/inventory/test_data/validate_query_parameter/data.json +++ b/tests/unit/inventory/test_data/validate_query_parameter/data.json @@ -1,14 +1,14 @@ [ { - "parameter": "a", + "parameter": "name", "expected": true }, { - "parameter": "b", + "parameter": "platform", "expected": true }, { - "parameter": "c", + "parameter": "region", "expected": true }, { diff --git a/tests/unit/inventory/test_nb_inventory.py b/tests/unit/inventory/test_nb_inventory.py index 66ccbd953..1111a720a 100644 --- a/tests/unit/inventory/test_nb_inventory.py +++ b/tests/unit/inventory/test_nb_inventory.py @@ -9,7 +9,7 @@ try: from ansible_collections.netbox.netbox.plugins.inventory.nb_inventory import ( - InventoryModule, + InventoryModule, ALLOWED_DEVICE_QUERY_PARAMETERS ) from ansible_collections.netbox.netbox.tests.test_data import load_test_data @@ -21,7 +21,7 @@ sys.path.append("plugins/inventory") sys.path.append("tests") - from nb_inventory import InventoryModule + from nb_inventory import InventoryModule, ALLOWED_DEVICE_QUERY_PARAMETERS from test_data import load_test_data load_relative_test_data = partial( @@ -38,20 +38,20 @@ def inventory_fixture(): @pytest.fixture -def allowed_query_parameters_fixture(): - return ["a", "b", "c"] +def allowed_device_query_parameters_fixture(): + return ALLOWED_DEVICE_QUERY_PARAMETERS @pytest.mark.parametrize( "parameter, expected", load_relative_test_data("validate_query_parameter") ) def test_validate_query_parameter( - inventory_fixture, allowed_query_parameters_fixture, parameter, expected + inventory_fixture, allowed_device_query_parameters_fixture, parameter, expected ): value = "some value, doesn't matter" result = inventory_fixture.validate_query_parameter( - {parameter: value}, allowed_query_parameters_fixture + {parameter: value}, allowed_device_query_parameters_fixture ) assert (result == (parameter, value)) == expected @@ -60,11 +60,11 @@ def test_validate_query_parameter( "parameters, expected", load_relative_test_data("filter_query_parameters") ) def test_filter_query_parameters( - inventory_fixture, allowed_query_parameters_fixture, parameters, expected + inventory_fixture, allowed_device_query_parameters_fixture, parameters, expected ): result = inventory_fixture.filter_query_parameters( - parameters, allowed_query_parameters_fixture + parameters, allowed_device_query_parameters_fixture ) # Result is iterators of tuples From 738ea197bb013743fc2035ba0ed0d126cfed5369 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Tue, 14 Apr 2020 13:30:17 +1000 Subject: [PATCH 09/10] Inventory: don't check types of query_filters options - ansible handles this for us already (#140) --- plugins/inventory/nb_inventory.py | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/plugins/inventory/nb_inventory.py b/plugins/inventory/nb_inventory.py index fffc8f4cb..4242a5fcd 100644 --- a/plugins/inventory/nb_inventory.py +++ b/plugins/inventory/nb_inventory.py @@ -909,30 +909,4 @@ def parse(self, inventory, loader, path, cache=True): self.device_query_filters = self.get_option("device_query_filters") self.vm_query_filters = self.get_option("vm_query_filters") - # Basic type checking of options - if self.query_filters != None and not isinstance(self.query_filters, Iterable): - raise AnsibleError( - "query_filters is set, but is not a list of dictionaries", - obj=self.query_filters, - show_content=True, - ) - - if self.device_query_filters != None and not isinstance( - self.device_query_filters, Iterable - ): - raise AnsibleError( - "device_query_filters is set, but is not a list of dictionaries", - obj=self.device_query_filters, - show_content=True, - ) - - if self.vm_query_filters != None and not isinstance( - self.vm_query_filters, Iterable - ): - raise AnsibleError( - "vm_query_filters is set, but is not a list of dictionaries", - obj=self.vm_query_filters, - show_content=True, - ) - self.main() From 54af4ed04c2de63bdff436c6a1ea6c4a9eff9ba7 Mon Sep 17 00:00:00 2001 From: Douglas Heriot Date: Tue, 14 Apr 2020 22:21:34 +1000 Subject: [PATCH 10/10] Inventory: black formatting changes (#140) --- tests/unit/inventory/test_nb_inventory.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/inventory/test_nb_inventory.py b/tests/unit/inventory/test_nb_inventory.py index 1111a720a..417585d1b 100644 --- a/tests/unit/inventory/test_nb_inventory.py +++ b/tests/unit/inventory/test_nb_inventory.py @@ -9,7 +9,8 @@ try: from ansible_collections.netbox.netbox.plugins.inventory.nb_inventory import ( - InventoryModule, ALLOWED_DEVICE_QUERY_PARAMETERS + InventoryModule, + ALLOWED_DEVICE_QUERY_PARAMETERS, ) from ansible_collections.netbox.netbox.tests.test_data import load_test_data