From 33754b467f291bc4f3b14767ed7e2917ec87c1fc Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 24 Apr 2022 04:13:28 -0700 Subject: [PATCH 01/15] Added vm filters --- src/saltext/vmware/modules/vm.py | 19 ++++++++++++++-- src/saltext/vmware/utils/vm.py | 39 ++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index a9f3599d..db05cb44 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -26,16 +26,31 @@ def __virtual__(): return __virtualname__ -def list_(service_instance=None): +def list_( + service_instance=None, + datacenter_name=None, + cluster_name=None +): """ Returns virtual machines. + datacenter_name + Filter by this datacenter name (required when cluster is specified) + + cluster_name + Filter by this cluster name (optional) + service_instance (optional) The Service Instance from which to obtain managed object references. """ + log.debug("Running vmware_vm.list") if service_instance is None: service_instance = connect.get_service_instance(opts=__opts__, pillar=__pillar__) - return utils_vm.list_vms(service_instance) + return utils_vm.list_vms( + service_instance=service_instance, + cluster_name=cluster_name, + datacenter_name=datacenter_name + ) def list_templates(service_instance=None): diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index a81aa6d4..c6c5627c 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -316,15 +316,50 @@ def unregister_vm(vm_ref): raise salt.exceptions.VMwareRuntimeError(exc.msg) -def list_vms(service_instance): +def list_vms( + service_instance, + datacenter_name=None, + cluster_name=None +): """ Returns a list of VMs associated with a given service instance. service_instance The Service Instance Object from which to obtain VMs. + + datacenter_name + The datacenter name. Default is None. + + cluster_name + The cluster name - used to restrict the VMs retrieved. Only used if + the datacenter is set. This argument is optional. """ + properties = ["name"] + + if cluster_name and not datacenter_name: + raise salt.exceptions.ArgumentValueError( + "Must specify the datacenter when specifying the cluster" + ) + + if not datacenter_name: + # Assume the root folder is the starting point + start_point = utils_common.get_root_folder(service_instance) + else: + start_point = utils_datacenter.get_datacenter(service_instance, datacenter_name) + if cluster_name: + # Retrieval to test if cluster exists. Cluster existence only makes + # sense if the datacenter has been specified + properties.append("parent") + + # Search for the objects + vms = utils_common.get_mors_with_properties( + service_instance, + vim.VirtualMachine, + container_ref=start_point, + property_list=properties, + ) + items = [] - vms = utils_common.get_mors_with_properties(service_instance, vim.VirtualMachine) for vm in vms: if not vm["config"].template: items.append(vm["name"]) From 74ad139d5cf188eddbc7dbbdbe56d48ebf13e17f Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 24 Apr 2022 06:22:49 -0700 Subject: [PATCH 02/15] Fixed vm property listing --- src/saltext/vmware/utils/vm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index c6c5627c..694799f0 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -334,7 +334,7 @@ def list_vms( The cluster name - used to restrict the VMs retrieved. Only used if the datacenter is set. This argument is optional. """ - properties = ["name"] + properties = ["name", "config"] if cluster_name and not datacenter_name: raise salt.exceptions.ArgumentValueError( From 0e7cca10b0713a438fdab4b1ce50ba7eaa41ddde Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 24 Apr 2022 07:20:48 -0700 Subject: [PATCH 03/15] Added vm filtering by host_name --- src/saltext/vmware/modules/vm.py | 7 ++++++- src/saltext/vmware/utils/vm.py | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index db05cb44..a6235e5f 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -29,7 +29,8 @@ def __virtual__(): def list_( service_instance=None, datacenter_name=None, - cluster_name=None + cluster_name=None, + host_name=None ): """ Returns virtual machines. @@ -40,6 +41,9 @@ def list_( cluster_name Filter by this cluster name (optional) + host_name + Filter by this host name (optional) + service_instance (optional) The Service Instance from which to obtain managed object references. """ @@ -48,6 +52,7 @@ def list_( service_instance = connect.get_service_instance(opts=__opts__, pillar=__pillar__) return utils_vm.list_vms( service_instance=service_instance, + host_name=host_name, cluster_name=cluster_name, datacenter_name=datacenter_name ) diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index 694799f0..85a4ecf6 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -319,7 +319,8 @@ def unregister_vm(vm_ref): def list_vms( service_instance, datacenter_name=None, - cluster_name=None + cluster_name=None, + host_name=None, ): """ Returns a list of VMs associated with a given service instance. @@ -333,6 +334,9 @@ def list_vms( cluster_name The cluster name - used to restrict the VMs retrieved. Only used if the datacenter is set. This argument is optional. + + host_name + The host_name to be retrieved. Default is None. """ properties = ["name", "config"] @@ -351,6 +355,15 @@ def list_vms( # sense if the datacenter has been specified properties.append("parent") + if host_name: + host = utils_common.get_mor_by_property( + service_instance, + vim.HostSystem, + host_name + ) + properties.append("runtime.host") + log.trace("Retrieved host: %s", host) + # Search for the objects vms = utils_common.get_mors_with_properties( service_instance, @@ -361,6 +374,11 @@ def list_vms( items = [] for vm in vms: + if host_name: + if host == vm["runtime.host"]: + items.append(vm["name"]) + else: + break if not vm["config"].template: items.append(vm["name"]) return items From 3abfb3c6546fdb0835a95ffc77d2ada49b6e2969 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 24 Apr 2022 11:49:09 -0700 Subject: [PATCH 04/15] Fixed logic check --- src/saltext/vmware/utils/vm.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index 85a4ecf6..979b5484 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -363,6 +363,8 @@ def list_vms( ) properties.append("runtime.host") log.trace("Retrieved host: %s", host) + else: + host = None # Search for the objects vms = utils_common.get_mors_with_properties( @@ -374,13 +376,11 @@ def list_vms( items = [] for vm in vms: - if host_name: - if host == vm["runtime.host"]: + if not vm["config"].template: + if host_name and host == vm["runtime.host"]: items.append(vm["name"]) else: - break - if not vm["config"].template: - items.append(vm["name"]) + items.append(vm["name"]) return items From fab419054cb790a80feaec1455b590f6c810c492 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 24 Apr 2022 13:20:59 -0700 Subject: [PATCH 05/15] Fixed logic check --- src/saltext/vmware/utils/vm.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index 979b5484..b83c2f3f 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -377,8 +377,9 @@ def list_vms( items = [] for vm in vms: if not vm["config"].template: - if host_name and host == vm["runtime.host"]: - items.append(vm["name"]) + if host_name: + if host == vm["runtime.host"]: + items.append(vm["name"]) else: items.append(vm["name"]) return items From 82721a1cc2c67df7127bd3aa64b0f417b13d36bb Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Tue, 26 Apr 2022 09:46:41 -0700 Subject: [PATCH 06/15] Fixed pre-commit issues --- src/saltext/vmware/modules/vm.py | 9 ++------- src/saltext/vmware/utils/vm.py | 24 ++++++++++-------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index a6235e5f..9a3e7852 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -26,12 +26,7 @@ def __virtual__(): return __virtualname__ -def list_( - service_instance=None, - datacenter_name=None, - cluster_name=None, - host_name=None -): +def list_(service_instance=None, datacenter_name=None, cluster_name=None, host_name=None): """ Returns virtual machines. @@ -54,7 +49,7 @@ def list_( service_instance=service_instance, host_name=host_name, cluster_name=cluster_name, - datacenter_name=datacenter_name + datacenter_name=datacenter_name, ) diff --git a/src/saltext/vmware/utils/vm.py b/src/saltext/vmware/utils/vm.py index b83c2f3f..4066af55 100644 --- a/src/saltext/vmware/utils/vm.py +++ b/src/saltext/vmware/utils/vm.py @@ -317,10 +317,10 @@ def unregister_vm(vm_ref): def list_vms( - service_instance, - datacenter_name=None, - cluster_name=None, - host_name=None, + service_instance, + datacenter_name=None, + cluster_name=None, + host_name=None, ): """ Returns a list of VMs associated with a given service instance. @@ -356,11 +356,7 @@ def list_vms( properties.append("parent") if host_name: - host = utils_common.get_mor_by_property( - service_instance, - vim.HostSystem, - host_name - ) + host = utils_common.get_mor_by_property(service_instance, vim.HostSystem, host_name) properties.append("runtime.host") log.trace("Retrieved host: %s", host) else: @@ -368,11 +364,11 @@ def list_vms( # Search for the objects vms = utils_common.get_mors_with_properties( - service_instance, - vim.VirtualMachine, - container_ref=start_point, - property_list=properties, - ) + service_instance, + vim.VirtualMachine, + container_ref=start_point, + property_list=properties, + ) items = [] for vm in vms: From 7d4564736e18fec596f58568fdf765d1219918c1 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Tue, 26 Apr 2022 16:00:28 -0700 Subject: [PATCH 07/15] Updated vm list test case --- tests/integration/modules/test_vm.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration/modules/test_vm.py b/tests/integration/modules/test_vm.py index 7a5a1e02..f55420e7 100644 --- a/tests/integration/modules/test_vm.py +++ b/tests/integration/modules/test_vm.py @@ -56,7 +56,10 @@ def test_vm_list(integration_test_config, patch_salt_globals_vm): """ Test vm list_ """ - all = virtual_machine.list_() + all = virtual_machine.list_( + datacenter_name="Datacenter", + cluster_name="Cluster", + ) assert all == integration_test_config["virtual_machines"] From 05534204d043c34bbb7f99119ab6c6e3e9a2fdaf Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Wed, 20 Jul 2022 14:21:10 -0500 Subject: [PATCH 08/15] start testing vm mod --- tests/unit/modules/test_vm.py | 111 ++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 tests/unit/modules/test_vm.py diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py new file mode 100644 index 00000000..35b2e994 --- /dev/null +++ b/tests/unit/modules/test_vm.py @@ -0,0 +1,111 @@ +from collections import namedtuple +import pytest +import salt.exceptions +import saltext.vmware.modules.vm as vm +import unittest.mock as mock + +# TODO: why is `name` bad when it comes to Mock? Why do we need to use a namedtuple? -W. Werner, 2022-07-19 +PropSet = namedtuple("PropSet", "name,val") + +@pytest.fixture +def fake_vmodl(): + with mock.patch("saltext.vmware.utils.common.vmodl") as fake_vmodl: + yield fake_vmodl + + +@pytest.mark.parametrize( + "datacenter_name", + [ + None, + "", + ], +) +def test_when_vm_list_is_called_with_cluster_name_but_no_datacenter_name_then_it_should_ArgumentValueError( + datacenter_name, +): + expected_message = "Must specify the datacenter when specifying the cluster" + with pytest.raises(salt.exceptions.ArgumentValueError, match=expected_message): + vm.list_( + service_instance="fnord", + datacenter_name=datacenter_name, + cluster_name="Anything that is not falsey", + ) + + +@pytest.fixture +def vm_property_contents(): + non_template_config = mock.Mock(template=None) + + fake_content_1 = mock.MagicMock() + fake_content_1.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="okay?"), + ] + fake_content_1.obj = "fnord" + + fake_content_2 = mock.MagicMock() + fake_content_2.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="foo"), + ] + fake_content_2.obj = "fnord" + + fake_content_3 = mock.MagicMock() + fake_content_3.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="bar"), + ] + fake_content_3.obj = "fnord" + + vm_names = ["okay?", "foo", "bar"] + return vm_names, [fake_content_1, fake_content_2, fake_content_3] + +@pytest.fixture +def template_property_contents(): + template_config = mock.Mock(template=True) + + fake_template_1 = mock.MagicMock() + fake_template_1.propSet = [ + PropSet(name="foo", val="foo"), + PropSet(name="bar", val="barval"), + PropSet(name="config", val=template_config), + PropSet(name="name", val="template 1"), + ] + fake_template_1.obj = "fnord" + + template_names = ["template 1"] + return template_names, [fake_template_1] + + +# TODO: Rename this -W. Werner, 2022-07-19 +@pytest.fixture +def quacks_like_vms(vm_property_contents, template_property_contents): + contents = [] + vm_names, vm_contents = vm_property_contents + template_names, template_contents = template_property_contents + contents.extend(vm_contents) + contents.extend(template_contents) + + fake_service_instance = mock.MagicMock() + fake_service_instance.content.propertyCollector.RetrieveContents.return_value = contents + return fake_service_instance, vm_names, template_names + + +def test_when_vm_list_is_not_given_a_datacenter_or_hostname_it_should_return_expected_vms( + fake_vmodl, quacks_like_vms +): + datacenter_name = None + host_name = None + fake_service_instance, expected_vm_names, _ = quacks_like_vms + + actual_vm_names = vm.list_( + service_instance=fake_service_instance, datacenter_name=datacenter_name, host_name=host_name + ) + + assert actual_vm_names == expected_vm_names From f4b60c5450751cf6a99ab317803e3e626800c623 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Thu, 21 Jul 2022 12:38:01 -0500 Subject: [PATCH 09/15] add another test --- tests/unit/modules/test_vm.py | 151 +++++++++++++++++++++++++++++----- 1 file changed, 129 insertions(+), 22 deletions(-) diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py index 35b2e994..7ea5900a 100644 --- a/tests/unit/modules/test_vm.py +++ b/tests/unit/modules/test_vm.py @@ -1,4 +1,5 @@ from collections import namedtuple +import itertools import pytest import salt.exceptions import saltext.vmware.modules.vm as vm @@ -6,42 +7,26 @@ # TODO: why is `name` bad when it comes to Mock? Why do we need to use a namedtuple? -W. Werner, 2022-07-19 PropSet = namedtuple("PropSet", "name,val") +non_template_config = mock.Mock(template=None) + @pytest.fixture def fake_vmodl(): with mock.patch("saltext.vmware.utils.common.vmodl") as fake_vmodl: + # TODO: Should we replace this with the actual vmodl.RuntimeFault? -W. Werner, 2022-07-21 + fake_vmodl.RuntimeFault = Exception yield fake_vmodl -@pytest.mark.parametrize( - "datacenter_name", - [ - None, - "", - ], -) -def test_when_vm_list_is_called_with_cluster_name_but_no_datacenter_name_then_it_should_ArgumentValueError( - datacenter_name, -): - expected_message = "Must specify the datacenter when specifying the cluster" - with pytest.raises(salt.exceptions.ArgumentValueError, match=expected_message): - vm.list_( - service_instance="fnord", - datacenter_name=datacenter_name, - cluster_name="Anything that is not falsey", - ) - - @pytest.fixture def vm_property_contents(): - non_template_config = mock.Mock(template=None) - fake_content_1 = mock.MagicMock() fake_content_1.propSet = [ PropSet(name="foo", val="foo"), PropSet(name="bar", val="barval"), PropSet(name="config", val=non_template_config), PropSet(name="name", val="okay?"), + PropSet(name="runtime.host", val="fnord"), ] fake_content_1.obj = "fnord" @@ -51,6 +36,7 @@ def vm_property_contents(): PropSet(name="bar", val="barval"), PropSet(name="config", val=non_template_config), PropSet(name="name", val="foo"), + PropSet(name="runtime.host", val="fnord"), ] fake_content_2.obj = "fnord" @@ -60,12 +46,14 @@ def vm_property_contents(): PropSet(name="bar", val="barval"), PropSet(name="config", val=non_template_config), PropSet(name="name", val="bar"), + PropSet(name="runtime.host", val="fnord"), ] fake_content_3.obj = "fnord" vm_names = ["okay?", "foo", "bar"] return vm_names, [fake_content_1, fake_content_2, fake_content_3] + @pytest.fixture def template_property_contents(): template_config = mock.Mock(template=True) @@ -83,6 +71,67 @@ def template_property_contents(): return template_names, [fake_template_1] +@pytest.fixture +def vms_with_coolguy_host(quacks_like_vms): + vm_names = ["coolguy", "cooldude", "coolbro"] + coolguy_host = object() + hosty_thing = mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + # This coolguy is the host.name referred to in get_mor_by_property + # or in other words, *this* is the name we search for when + # host_name is provided. + PropSet(name="name", val="coolguy"), + ], + ) + hosty_thing.obj = coolguy_host + + # The PropSet `name` should align with `vm_names`. Yes, normally we would + # just iterate over the vm_names, but when testing that can accidentally + # turn into not actually testing anything. Better to just manually do + # things here. + vms = [ + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="coolguy"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="cooldude"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + mock.MagicMock( + propSet=[ + PropSet(name="ignore", val="me"), + PropSet(name="something", val="irrelevant"), + PropSet(name="config", val=non_template_config), + PropSet(name="name", val="coolbro"), + PropSet(name="runtime.host", val=coolguy_host), + ], + ), + ] + si, *_ = quacks_like_vms + # We can't just insert stuff into the mock's `side_effect` willy-nilly. + # Instead we have to replace the side_effect with our new one. For some + # reason we have to expand with `*si.content...` the existing side_effect + # when we want to itertools.chain it. And since the existing side effect is + # our vms, we need to adjust the side_effect this way. + si.content.propertyCollector.RetrieveContents.side_effect = [ + [hosty_thing], + itertools.chain(*si.content.propertyCollector.RetrieveContents.side_effect, vms), + ] + return coolguy_host, vm_names + + # TODO: Rename this -W. Werner, 2022-07-19 @pytest.fixture def quacks_like_vms(vm_property_contents, template_property_contents): @@ -93,10 +142,39 @@ def quacks_like_vms(vm_property_contents, template_property_contents): contents.extend(template_contents) fake_service_instance = mock.MagicMock() - fake_service_instance.content.propertyCollector.RetrieveContents.return_value = contents + fake_service_instance.content.propertyCollector.RetrieveContents.side_effect = [contents] return fake_service_instance, vm_names, template_names +@pytest.mark.parametrize( + "datacenter_name", + [ + # To be fair, only None or "" are common - but to be comprehensive, + # let's go ahead and test the rest of the False-y things. + None, + "", + # Yep, anything below here is silly, but also still behaves the same + # way + [], + (), + {}, + False, + 0, + 0.0, + ], +) +def test_when_vm_list_is_called_with_cluster_name_but_no_datacenter_name_then_it_should_ArgumentValueError( + datacenter_name, +): + expected_message = "Must specify the datacenter when specifying the cluster" + with pytest.raises(salt.exceptions.ArgumentValueError, match=expected_message): + vm.list_( + service_instance="fnord", + datacenter_name=datacenter_name, + cluster_name="Anything that is not falsey", + ) + + def test_when_vm_list_is_not_given_a_datacenter_or_hostname_it_should_return_expected_vms( fake_vmodl, quacks_like_vms ): @@ -109,3 +187,32 @@ def test_when_vm_list_is_not_given_a_datacenter_or_hostname_it_should_return_exp ) assert actual_vm_names == expected_vm_names + + +def test_when_vm_list_is_given_a_hostname_then_only_vms_with_matching_runtime_host_should_be_returned( + fake_vmodl, quacks_like_vms, vms_with_coolguy_host +): + coolguy_host, expected_vm_names = vms_with_coolguy_host + fake_service_instance, _, _ = quacks_like_vms + actual_vm_names = vm.list_(service_instance=fake_service_instance, host_name="coolguy") + assert actual_vm_names == expected_vm_names + + + +def blah(thing): + f = thing.fnord() + assert f == 42 + + blah = thing.fnord() + assert blah == 5 + + thing.fnord() + + +def test_whee(): + m = mock.MagicMock() + + #m.fnord.return_value = 42 + m.fnord.side_effect = [42, 5] + + blah(m) From db0320ca4a857501dc6d1ffd8fe01a0f77dfdc85 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 2 Aug 2022 15:06:37 -0500 Subject: [PATCH 10/15] finish up tests --- tests/unit/modules/test_vm.py | 52 ++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py index 7ea5900a..66ae9ad4 100644 --- a/tests/unit/modules/test_vm.py +++ b/tests/unit/modules/test_vm.py @@ -198,21 +198,47 @@ def test_when_vm_list_is_given_a_hostname_then_only_vms_with_matching_runtime_ho assert actual_vm_names == expected_vm_names +def test_when_vm_list_is_given_a_cluster_name_and_datacenter_name_then_parent_should_be_added_to_filter_properties( + fake_vmodl, quacks_like_vms +): + fake_service_instance, *_ = quacks_like_vms + with mock.patch( + "saltext.vmware.utils.common.get_datacenters", return_value=["fnord"], autospec=True + ): + vm.list_( + service_instance=fake_service_instance, + datacenter_name="fnord as well", + cluster_name="anything not false/empty", + ) + assert ( + "parent" in fake_vmodl.query.PropertyCollector.PropertySpec.mock_calls[0].kwargs["pathSet"] + ) -def blah(thing): - f = thing.fnord() - assert f == 42 - - blah = thing.fnord() - assert blah == 5 - - thing.fnord() +def test_when_vm_list_is_given_a_datacenter_name_but_no_cluster_name_then_it_should_return_expected_vms_as_filtered_by_datacenter( + fake_vmodl, quacks_like_vms +): + fake_datacenter = object() + fake_service_instance, expected_vm_names, _ = quacks_like_vms + with mock.patch( + "saltext.vmware.utils.common.get_datacenters", return_value=[fake_datacenter], autospec=True + ): + actual_vm_names = vm.list_( + service_instance=fake_service_instance, datacenter_name="some cool datacenter" + ) -def test_whee(): - m = mock.MagicMock() + assert actual_vm_names == expected_vm_names - #m.fnord.return_value = 42 - m.fnord.side_effect = [42, 5] + # Yes, these assertions are kind of gross - but we're dealing with pyvmomi and all that that entails. I don't think that there's really a great way to make these assertions. + assert ( + fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0].args[0] + is fake_datacenter + ) - blah(m) + assert ( + fake_vmodl.query.PropertyCollector.ObjectSpec.mock_calls[0].kwargs["obj"] + == fake_service_instance.content.viewManager.CreateContainerView.return_value + ) + assert fake_vmodl.query.PropertyCollector.FilterSpec.mock_calls[0].kwargs["objectSet"] == [ + fake_vmodl.query.PropertyCollector.ObjectSpec.return_value + ] From 2f70db148c565bb37075bfbb86796e549c10fcd5 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Fri, 30 Sep 2022 01:06:47 +0530 Subject: [PATCH 11/15] fixed lint issue --- src/saltext/vmware/modules/vm.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index 8ac71789..839ac6a7 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -26,7 +26,9 @@ def __virtual__(): return __virtualname__ -def list_(service_instance=None, datacenter_name=None, cluster_name=None, host_name=None, profile=None): +def list_( + service_instance=None, datacenter_name=None, cluster_name=None, host_name=None, profile=None +): """ Returns virtual machines. From 26507e52d126ba310c6b03645adfccdf889a8618 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Wed, 9 Nov 2022 22:15:18 +0530 Subject: [PATCH 12/15] fix pre-commit issues --- tests/unit/modules/test_vm.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py index 66ae9ad4..a6fd5fb1 100644 --- a/tests/unit/modules/test_vm.py +++ b/tests/unit/modules/test_vm.py @@ -1,9 +1,10 @@ -from collections import namedtuple import itertools +import unittest.mock as mock +from collections import namedtuple + import pytest import salt.exceptions import saltext.vmware.modules.vm as vm -import unittest.mock as mock # TODO: why is `name` bad when it comes to Mock? Why do we need to use a namedtuple? -W. Werner, 2022-07-19 PropSet = namedtuple("PropSet", "name,val") From 85da422733f3837e14e84cba47658b75f05c4a37 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Thu, 10 Nov 2022 21:47:01 +0530 Subject: [PATCH 13/15] fix pre-commit issues --- src/saltext/vmware/modules/vm.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/saltext/vmware/modules/vm.py b/src/saltext/vmware/modules/vm.py index 35932555..d441eb6d 100644 --- a/src/saltext/vmware/modules/vm.py +++ b/src/saltext/vmware/modules/vm.py @@ -55,7 +55,9 @@ def list_( salt '*' vmware_vm.list """ log.debug("Running vmware_vm.list") - service_instance = service_instance or connect.get_service_instance(config=__opts__, profile=profile) + service_instance = service_instance or connect.get_service_instance( + config=__opts__, profile=profile + ) return utils_vm.list_vms( service_instance=service_instance, host_name=host_name, From 20a210f5af7db61a385990154ba7e5802b857c20 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 15 Nov 2022 18:10:04 -0600 Subject: [PATCH 14/15] update to latest version of python setup in actions --- .github/workflows/test.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0c91ad52..cdcfe404 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -9,7 +9,7 @@ jobs: steps: - uses: actions/checkout@v2 - name: Set up Python - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: 3.7 - name: Set Cache Key @@ -34,7 +34,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python 3.7 For Nox - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: 3.7 @@ -74,7 +74,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} @@ -192,7 +192,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} @@ -315,7 +315,7 @@ jobs: - uses: actions/checkout@v2 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v1 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} From f3e20f2699e52903fa90afded8c6ad5e60b2316d Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 15 Nov 2022 19:02:10 -0600 Subject: [PATCH 15/15] make tests pass on 3.7 --- tests/unit/modules/test_vm.py | 47 ++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/tests/unit/modules/test_vm.py b/tests/unit/modules/test_vm.py index a6fd5fb1..0b4d2bb4 100644 --- a/tests/unit/modules/test_vm.py +++ b/tests/unit/modules/test_vm.py @@ -1,4 +1,5 @@ import itertools +import sys import unittest.mock as mock from collections import namedtuple @@ -211,9 +212,19 @@ def test_when_vm_list_is_given_a_cluster_name_and_datacenter_name_then_parent_sh datacenter_name="fnord as well", cluster_name="anything not false/empty", ) - assert ( - "parent" in fake_vmodl.query.PropertyCollector.PropertySpec.mock_calls[0].kwargs["pathSet"] - ) + + if sys.version_info < (3, 8): + from pyVmomi import vim + + expected_call = mock.call( + all=False, pathSet=["name", "config", "parent"], type=vim.VirtualMachine + ) + assert fake_vmodl.query.PropertyCollector.PropertySpec.has_call(expected_call) + else: + assert ( + "parent" + in fake_vmodl.query.PropertyCollector.PropertySpec.mock_calls[0].kwargs["pathSet"] + ) def test_when_vm_list_is_given_a_datacenter_name_but_no_cluster_name_then_it_should_return_expected_vms_as_filtered_by_datacenter( @@ -230,16 +241,22 @@ def test_when_vm_list_is_given_a_datacenter_name_but_no_cluster_name_then_it_sho assert actual_vm_names == expected_vm_names - # Yes, these assertions are kind of gross - but we're dealing with pyvmomi and all that that entails. I don't think that there's really a great way to make these assertions. - assert ( - fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0].args[0] - is fake_datacenter - ) + if sys.version_info < (3, 8): + assert ( + fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0][1][0] + is fake_datacenter + ) + else: + # Yes, these assertions are kind of gross - but we're dealing with pyvmomi and all that that entails. I don't think that there's really a great way to make these assertions. + assert ( + fake_service_instance.content.viewManager.CreateContainerView.mock_calls[0].args[0] + is fake_datacenter + ) - assert ( - fake_vmodl.query.PropertyCollector.ObjectSpec.mock_calls[0].kwargs["obj"] - == fake_service_instance.content.viewManager.CreateContainerView.return_value - ) - assert fake_vmodl.query.PropertyCollector.FilterSpec.mock_calls[0].kwargs["objectSet"] == [ - fake_vmodl.query.PropertyCollector.ObjectSpec.return_value - ] + assert ( + fake_vmodl.query.PropertyCollector.ObjectSpec.mock_calls[0].kwargs["obj"] + == fake_service_instance.content.viewManager.CreateContainerView.return_value + ) + assert fake_vmodl.query.PropertyCollector.FilterSpec.mock_calls[0].kwargs["objectSet"] == [ + fake_vmodl.query.PropertyCollector.ObjectSpec.return_value + ]