From 08cbfc2fb2c7a1860411d91d93b443ff937979d1 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 11:44:32 -1000 Subject: [PATCH 1/7] BootflashFiles: address tech debt This commit addresses tech debt in class BootflashFile. The main changes are: - Remove class decorators in favor of class properties - Simplify logic in refresh_switch_details - Update unit-tests to align with changes Details 1. Remove class decorators 1a. Properties.add_rest_send 1b. Properties.add_results 2. Remove import for Properties 3. Add class properties for RestSend and Results 4. Import Results and RestSend for use in validation in their respective properties 5. import Typing.Literal for use in rest_send and results properties. 6. BootflashFiles.refresh_switch_details 6a. Diff lines 150-168, because rest_send validation occurs in the setter and getter, we can remove validation from refresh_switch_details and simplify this method. 7. test_bootflash_files.py 7a. test_bootflash_files_00000 - add asserts for rest_send and results 7b. test_bootflash_files_00100 - modify assert for earlier validation of rest_send 7c. test_bootflash_files_00110. modify assert for earlier validation of rest_send 7c. test_bootflash_files_00120. Remove. Results() is now initialized in BootflashFiles.__init__() --- .../module_utils/bootflash/bootflash_files.py | 102 +++++++++++++++--- .../dcnm_bootflash/test_bootflash_files.py | 40 ++----- 2 files changed, 95 insertions(+), 47 deletions(-) diff --git a/plugins/module_utils/bootflash/bootflash_files.py b/plugins/module_utils/bootflash/bootflash_files.py index d74f35db9..66407bf08 100644 --- a/plugins/module_utils/bootflash/bootflash_files.py +++ b/plugins/module_utils/bootflash/bootflash_files.py @@ -19,15 +19,15 @@ import copy import inspect import logging +from typing import Literal from ..common.api.v1.imagemanagement.rest.imagemgnt.bootflash.bootflash import \ EpBootflashFiles from ..common.conversion import ConversionUtils -from ..common.properties import Properties +from ..common.results import Results +from ..common.rest_send_v2 import RestSend -@Properties.add_rest_send -@Properties.add_results class BootflashFiles: """ ### Summary @@ -131,8 +131,8 @@ def __init__(self): self._filepath = None self._ip_address = None self._partition = None - self._rest_send = None - self._results = None + self._rest_send: RestSend = RestSend({}) + self._results: Results = Results() self._supervisor = None self._switch_details = None self._target = None @@ -154,17 +154,11 @@ def refresh_switch_details(self): """ method_name = inspect.stack()[0][3] - def raise_exception(property_name): + if self.switch_details is None: msg = f"{self.class_name}.{method_name}: " - msg += f"{property_name} must be set before calling {method_name}." + msg += f"switch_details must be set before calling {method_name}." raise ValueError(f"{msg}") - if self.switch_details is None: - raise_exception("switch_details") - # pylint: disable=no-member - if self.rest_send is None: - raise_exception("rest_send") - if self.switch_details_refreshed is False: self.switch_details.rest_send = self.rest_send self.switch_details.refresh() @@ -584,6 +578,88 @@ def partition(self): def partition(self, value): self._partition = value + @property + def rest_send(self): + """ + # Summary + + An instance of the RestSend class. + + ## Raises + + - setter: `TypeError` if the value is not an instance of RestSend. + - setter: `ValueError` if RestSend.params is not set. + + ## getter + + Return an instance of the RestSend class. + + ## setter + + Set an instance of the RestSend class. + """ + method_name: str = inspect.stack()[0][3] + if not self._rest_send.params: + msg = f"{self.class_name}.{method_name}: " + msg += "RestSend.params must be set before accessing." + raise ValueError(msg) + return self._rest_send + + @rest_send.setter + def rest_send(self, value: RestSend) -> None: + method_name: str = inspect.stack()[0][3] + _class_have: str = "" + _class_need: Literal["RestSend"] = "RestSend" + msg = f"{self.class_name}.{method_name}: " + msg += f"value must be an instance of {_class_need}. " + msg += f"Got value {value} of type {type(value).__name__}." + try: + _class_have = value.class_name + except AttributeError as error: + msg += f" Error detail: {error}." + raise TypeError(msg) from error + if _class_have != _class_need: + raise TypeError(msg) + self._rest_send = value + + @property + def results(self) -> Results: + """ + # Summary + + An instance of the Results class. + + ## Raises + + - setter: `TypeError` if the value is not an instance of Results. + + ## getter + + Return an instance of the Results class. + + ## setter + + Set an instance of the Results class. + """ + return self._results + + @results.setter + def results(self, value: Results) -> None: + method_name: str = inspect.stack()[0][3] + _class_have: str = "" + _class_need: Literal["Results"] = "Results" + msg = f"{self.class_name}.{method_name}: " + msg += f"value must be an instance of {_class_need}. " + msg += f"Got value {value} of type {type(value).__name__}." + try: + _class_have = value.class_name + except AttributeError as error: + msg += f" Error detail: {error}." + raise TypeError(msg) from error + if _class_have != _class_need: + raise TypeError(msg) + self._results = value + @property def supervisor(self): """ diff --git a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py index a969e9d44..ea7904e7b 100644 --- a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py +++ b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py @@ -83,8 +83,9 @@ def test_bootflash_files_00000() -> None: assert instance.filepath is None assert instance.ip_address is None assert instance.partition is None - assert instance._rest_send is None - assert instance._results is None + assert instance._rest_send.params == {} + assert instance._rest_send.class_name == "RestSend" + assert instance._results.class_name == "Results" assert instance.supervisor is None assert instance.switch_details is None assert instance.target is None @@ -172,6 +173,7 @@ def responses(): instance.commit() assert instance.payload == payloads_bootflash_files(f"{key}a") + assert instance.results.response_current is not None assert instance.results.response_current["RETURN_CODE"] == 200 assert instance.results.result == [ {"success": True, "changed": True, "sequence_number": 1} @@ -195,37 +197,9 @@ def test_bootflash_files_00110() -> None: """ with does_not_raise(): instance = BootflashFiles() - instance.results = Results() instance.switch_details = SwitchDetails() - match = r"BootflashFiles.validate_commit_parameters:\s+" - match += r"rest_send must be set before calling commit\(\)\." - with pytest.raises(ValueError, match=match): - instance.commit() - - -def test_bootflash_files_00120() -> None: - """ - ### Classes and Methods - - BootflashFiles() - - commit() - - validate_commit_parameters() - - ### Summary - Verify ``ValueError`` is raised if ``results`` is not set before - calling commit. - - ### Test - - ValueError is raised by validate_commit_parameters(). - - Error message matches expectation. - """ - with does_not_raise(): - instance = BootflashFiles() - instance.rest_send = RestSend(params_deleted) - instance.switch_details = SwitchDetails() - - match = r"BootflashFiles.validate_commit_parameters:\s+" - match += r"results must be set before calling commit\(\)\." + match = r"BootflashFiles\.rest_send: RestSend.params must be set before accessing\." with pytest.raises(ValueError, match=match): instance.commit() @@ -725,9 +699,7 @@ def test_bootflash_files_00310() -> None: instance = BootflashFiles() instance.switch_details = SwitchDetails() - match = r"BootflashFiles\.refresh_switch_details:\s+" - match += r"rest_send must be set before calling\s+" - match += r"refresh_switch_details\." + match = r"BootflashFiles\.rest_send: RestSend.params must be set before accessing\." with pytest.raises(ValueError, match=match): instance.refresh_switch_details() From aed1ae9071949d419d86300f2d770057a92aec87 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 11:47:29 -1000 Subject: [PATCH 2/7] UT: Appease linters No functional changes in this commit. 1. Add pylint directive to suppress invalid-name 2 black reformating lines for more relaxed line length --- .../dcnm_bootflash/test_bootflash_files.py | 50 ++++++++----------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py index ea7904e7b..e83d1ba77 100644 --- a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py +++ b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py @@ -20,7 +20,7 @@ from __future__ import absolute_import, division, print_function -__metaclass__ = type +__metaclass__ = type # pylint: disable=invalid-name __copyright__ = "Copyright (c) 2024 Cisco and/or its affiliates." __author__ = "Allen Robel" @@ -29,26 +29,24 @@ import inspect import pytest -from ansible_collections.cisco.dcnm.plugins.module_utils.bootflash.bootflash_files import \ - BootflashFiles -from ansible_collections.cisco.dcnm.plugins.module_utils.bootflash.convert_target_to_params import \ - ConvertTargetToParams -from ansible_collections.cisco.dcnm.plugins.module_utils.common.response_handler import \ - ResponseHandler -from ansible_collections.cisco.dcnm.plugins.module_utils.common.rest_send_v2 import \ - RestSend -from ansible_collections.cisco.dcnm.plugins.module_utils.common.results import \ - Results -from ansible_collections.cisco.dcnm.plugins.module_utils.common.sender_file import \ - Sender -from ansible_collections.cisco.dcnm.plugins.module_utils.common.switch_details import \ - SwitchDetails -from ansible_collections.cisco.dcnm.tests.unit.module_utils.common.common_utils import \ - ResponseGenerator +from ansible_collections.cisco.dcnm.plugins.module_utils.bootflash.bootflash_files import BootflashFiles +from ansible_collections.cisco.dcnm.plugins.module_utils.bootflash.convert_target_to_params import ConvertTargetToParams +from ansible_collections.cisco.dcnm.plugins.module_utils.common.response_handler import ResponseHandler +from ansible_collections.cisco.dcnm.plugins.module_utils.common.rest_send_v2 import RestSend +from ansible_collections.cisco.dcnm.plugins.module_utils.common.results import Results +from ansible_collections.cisco.dcnm.plugins.module_utils.common.sender_file import Sender +from ansible_collections.cisco.dcnm.plugins.module_utils.common.switch_details import SwitchDetails +from ansible_collections.cisco.dcnm.tests.unit.module_utils.common.common_utils import ResponseGenerator from ansible_collections.cisco.dcnm.tests.unit.modules.dcnm.dcnm_bootflash.utils import ( - MockAnsibleModule, configs_deleted, does_not_raise, params_deleted, - payloads_bootflash_files, responses_ep_all_switches, - responses_ep_bootflash_files, targets) + MockAnsibleModule, + configs_deleted, + does_not_raise, + params_deleted, + payloads_bootflash_files, + responses_ep_all_switches, + responses_ep_bootflash_files, + targets, +) def test_bootflash_files_00000() -> None: @@ -175,9 +173,7 @@ def responses(): assert instance.payload == payloads_bootflash_files(f"{key}a") assert instance.results.response_current is not None assert instance.results.response_current["RETURN_CODE"] == 200 - assert instance.results.result == [ - {"success": True, "changed": True, "sequence_number": 1} - ] + assert instance.results.result == [{"success": True, "changed": True, "sequence_number": 1}] def test_bootflash_files_00110() -> None: @@ -795,9 +791,7 @@ def test_bootflash_files_00500() -> None: assert instance.results.response_current["RETURN_CODE"] == 200 assert instance.results.response_current["MESSAGE"] == "No files to delete." - assert instance.results.result == [ - {"success": True, "changed": False, "sequence_number": 1} - ] + assert instance.results.result == [{"success": True, "changed": False, "sequence_number": 1}] def test_bootflash_files_00600() -> None: @@ -973,9 +967,7 @@ def test_bootflash_files_00800() -> None: instance.target = "foo" -@pytest.mark.parametrize( - "parameter", ["filepath", "ip_address", "serial_number", "supervisor"] -) +@pytest.mark.parametrize("parameter", ["filepath", "ip_address", "serial_number", "supervisor"]) def test_bootflash_files_00810(parameter) -> None: """ ### Classes and Methods From 07588311e3db04ea0c112526b01fd1bf182b941a Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 11:48:35 -1000 Subject: [PATCH 3/7] UT: Update copyright dates No functional changs in this commit. --- .../unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py index e83d1ba77..463d27118 100644 --- a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py +++ b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py @@ -1,4 +1,4 @@ -# Copyright (c) 2024 Cisco and/or its affiliates. +# Copyright (c) 2024-2025 Cisco and/or its affiliates. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -22,7 +22,7 @@ __metaclass__ = type # pylint: disable=invalid-name -__copyright__ = "Copyright (c) 2024 Cisco and/or its affiliates." +__copyright__ = "Copyright (c) 2024-2025 Cisco and/or its affiliates." __author__ = "Allen Robel" import copy From 96ad742e786a83ebc4481a1a8b41b0f3f3f3d389 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 11:52:08 -1000 Subject: [PATCH 4/7] BootflashFiles: Appease linters No functional changes in this commit 1. Add pylint directive to suppress invalid-name 2 black reformating lines for more relaxed line length --- .../module_utils/bootflash/bootflash_files.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/plugins/module_utils/bootflash/bootflash_files.py b/plugins/module_utils/bootflash/bootflash_files.py index 66407bf08..a344032aa 100644 --- a/plugins/module_utils/bootflash/bootflash_files.py +++ b/plugins/module_utils/bootflash/bootflash_files.py @@ -13,7 +13,7 @@ # limitations under the License. from __future__ import absolute_import, division, print_function -__metaclass__ = type +__metaclass__ = type # pylint: disable=invalid-name __author__ = "Allen Robel" import copy @@ -21,11 +21,10 @@ import logging from typing import Literal -from ..common.api.v1.imagemanagement.rest.imagemgnt.bootflash.bootflash import \ - EpBootflashFiles +from ..common.api.v1.imagemanagement.rest.imagemgnt.bootflash.bootflash import EpBootflashFiles from ..common.conversion import ConversionUtils -from ..common.results import Results from ..common.rest_send_v2 import RestSend +from ..common.results import Results class BootflashFiles: @@ -267,9 +266,7 @@ def delete_files(self): self.rest_send.verb = self.ep_bootflash_files.verb self.rest_send.payload = self.payload self.rest_send.commit() - self.results.response_current = copy.deepcopy( - self.rest_send.response_current - ) + self.results.response_current = copy.deepcopy(self.rest_send.response_current) self.results.result_current = copy.deepcopy(self.rest_send.result_current) else: self.results.result_current = {"success": True, "changed": False} @@ -410,10 +407,7 @@ def add_file_to_existing_payload(self): continue files = item.get("files") for file in files: - if ( - file.get("fileName") == self.filename - and file.get("bootflashType") == self.supervisor - ): + if file.get("fileName") == self.filename and file.get("bootflashType") == self.supervisor: return files.append( { From 0568dfe586e22def9cbf2d7c9cd2e331ebc800b0 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 14:04:52 -1000 Subject: [PATCH 5/7] BootflashFiles: type-hints, validation hardening 1. Add type-hints 2. Add two methods to verify proper instantiation of rest_send and switch_details - _rest_send_instantiated - _switch_details_instantiated 3. add_file_to_existing_payload - Simplify logic 4. test_bootflash_files.py - Update all unit tests to reflect changes to BootflashFiles. - Some unit-tests started to fail because item 2 improved detection of improperly-validated RestSend and SwitchDetails. These tests were fixed. Mainly this had to do with switch_details.rest_send not being set. --- .../module_utils/bootflash/bootflash_files.py | 226 +++++++++++------- .../dcnm_bootflash/test_bootflash_files.py | 77 +++--- 2 files changed, 187 insertions(+), 116 deletions(-) diff --git a/plugins/module_utils/bootflash/bootflash_files.py b/plugins/module_utils/bootflash/bootflash_files.py index a344032aa..548d8302c 100644 --- a/plugins/module_utils/bootflash/bootflash_files.py +++ b/plugins/module_utils/bootflash/bootflash_files.py @@ -19,12 +19,13 @@ import copy import inspect import logging -from typing import Literal +from typing import Any, Literal from ..common.api.v1.imagemanagement.rest.imagemgnt.bootflash.bootflash import EpBootflashFiles from ..common.conversion import ConversionUtils from ..common.rest_send_v2 import RestSend from ..common.results import Results +from ..common.switch_details import SwitchDetails class BootflashFiles: @@ -106,42 +107,44 @@ class BootflashFiles: ``` """ - def __init__(self): - self.class_name = self.__class__.__name__ + def __init__(self) -> None: + self.class_name: str = self.__class__.__name__ - self.action = "bootflash_delete" - self.conversion = ConversionUtils() + self.action: Literal["bootflash_delete"] = "bootflash_delete" + self.conversion: ConversionUtils = ConversionUtils() # self.diff is keyed on switch ip_address and is updated # in self.update_diff(). - self.diff = {} - self.ep_bootflash_files = EpBootflashFiles() + self.diff: dict[str, Any] = {} + self.ep_bootflash_files: EpBootflashFiles = EpBootflashFiles() - self.ok_to_delete_files_reason = None - self.mandatory_target_keys = [ + self.ok_to_delete_files_reason: str = "" + self.mandatory_target_keys: list[str] = [ "filepath", "ip_address", "serial_number", "supervisor", ] - self.payload = {"deleteFiles": []} - self.switch_details_refreshed = False + self.payload: dict[str, list[dict[str, Any]]] = {"deleteFiles": []} + self.switch_details_refreshed: bool = False - self._filename = None - self._filepath = None - self._ip_address = None - self._partition = None + self._filename: str = "" + self._filepath: str = "" + self._ip_address: str = "" + self._partition: str = "" self._rest_send: RestSend = RestSend({}) self._results: Results = Results() - self._supervisor = None - self._switch_details = None - self._target = None + self._supervisor: str = "" + self._switch_details: SwitchDetails = SwitchDetails() + self._switch_details.results = Results() + self._switch_details.rest_send = RestSend({}) + self._target: dict[str, str] = {} self.log = logging.getLogger(f"dcnm.{self.class_name}") msg = "ENTERED BootflashQuery(): " msg += f"action {self.action}, " self.log.debug(msg) - def refresh_switch_details(self): + def refresh_switch_details(self) -> None: """ ### Summary If switch details are not already refreshed, refresh them. @@ -151,9 +154,13 @@ def refresh_switch_details(self): - ``switch_details`` is not set. - ``rest_send`` is not set. """ - method_name = inspect.stack()[0][3] + method_name: str = inspect.stack()[0][3] - if self.switch_details is None: + if not self._rest_send_instantiated(): + msg = f"{self.class_name}.{method_name}: " + msg += f"rest_send must be set before calling {method_name}." + raise ValueError(f"{msg}") + if not self._switch_details_instantiated(): msg = f"{self.class_name}.{method_name}: " msg += f"switch_details must be set before calling {method_name}." raise ValueError(f"{msg}") @@ -163,7 +170,7 @@ def refresh_switch_details(self): self.switch_details.refresh() self.switch_details_refreshed = True - def ip_address_to_serial_number(self, ip_address): + def ip_address_to_serial_number(self, ip_address: str) -> str: """ ### Summary Convert ip_address to serial_number. @@ -172,7 +179,7 @@ def ip_address_to_serial_number(self, ip_address): - ``ValueError`` if: - switch_details is not set. """ - method_name = inspect.stack()[0][3] + method_name: str = inspect.stack()[0][3] self.refresh_switch_details() @@ -185,7 +192,7 @@ def ip_address_to_serial_number(self, ip_address): raise ValueError(msg) from error return serial_number - def ok_to_delete_files(self, ip_address): + def ok_to_delete_files(self, ip_address: str) -> bool: """ ### Summary - Return True if files can be deleted on the switch with ip_address. @@ -203,34 +210,69 @@ def ok_to_delete_files(self, ip_address): return False return True + def _rest_send_instantiated(self) -> bool: + """ + # Summary + + - Return True if rest_send is properly instantiated. + - Return False otherwise. + + ## Raises + + None + """ + try: + if not self._rest_send.params: + return False + except AttributeError: + return False + return True + + def _switch_details_instantiated(self) -> bool: + """ + # Summary + + - Return True if switch_details is properly instantiated. + - Return False otherwise. + + ## Raises + + None + """ + try: + if not self._switch_details.rest_send.params: + return False + except AttributeError: + return False + return True + def validate_commit_parameters(self) -> None: """ - ### Summary + # Summary + Verify that mandatory prerequisites are met before calling commit. - ### Raises - - ``ValueError`` if: - - rest_send is not set. - - results is not set. - - switch_details is not set. - - payload is not set. + ## Raises + + ### ValueError + + - rest_send is not properly initialized. + - switch_details is not properly initialized. """ # pylint: disable=no-member - method_name = inspect.stack()[0][3] + method_name: str = inspect.stack()[0][3] - def raise_exception(property_name): + def raise_exception(property_name: str) -> None: msg = f"{self.class_name}.{method_name}: " msg += f"{property_name} must be set before calling commit()." raise ValueError(f"{msg}") - if not self.rest_send: + if not self._rest_send_instantiated(): raise_exception("rest_send") - if not self.results: - raise_exception("results") - if not self.switch_details: + if not self._switch_details_instantiated(): raise_exception("switch_details") - def commit(self): + def commit(self) -> None: """ ### Summary Send the payload to delete files. @@ -252,7 +294,7 @@ def commit(self): self.delete_files() - def delete_files(self): + def delete_files(self) -> None: """ ### Summary Delete files that have been added with add_files(). @@ -278,7 +320,7 @@ def delete_files(self): self.results.diff_current = copy.deepcopy(self.diff) self.results.register_task_result() - def validate_prerequisites_for_add_file(self): + def validate_prerequisites_for_add_file(self) -> None: """ ### Summary Verify that mandatory prerequisites are met before calling add_file() @@ -292,9 +334,9 @@ def validate_prerequisites_for_add_file(self): - ``switch_details`` is not set. - ``target`` is not set. """ - method_name = inspect.stack()[0][3] + method_name: str = inspect.stack()[0][3] - def raise_exception(property_name): + def raise_exception(property_name: str) -> None: msg = f"{self.class_name}.{method_name}: " msg += f"{property_name} must be set before calling add_file()." raise ValueError(f"{msg}") @@ -307,12 +349,12 @@ def raise_exception(property_name): raise_exception("ip_address") if not self.supervisor: raise_exception("supervisor") - if not self.switch_details: + if not self._switch_details_instantiated(): raise_exception("switch_details") if not self.target: raise_exception("target") - def partition_and_serial_number_exist_in_payload(self): + def partition_and_serial_number_exist_in_payload(self) -> bool: """ ### Summary - Return True if the partition and serialNumber associated with the @@ -349,10 +391,10 @@ def partition_and_serial_number_exist_in_payload(self): } ] """ - found = False + found: bool = False for item in self.payload["deleteFiles"]: - serial_number = item.get("serialNumber") - partition = item.get("partition") + serial_number: str = item.get("serialNumber", "") + partition: str = item.get("partition", "") if serial_number != self.ip_address_to_serial_number(self.ip_address): continue if partition != self.partition: @@ -361,19 +403,23 @@ def partition_and_serial_number_exist_in_payload(self): break return found - def add_file_to_existing_payload(self): + def add_file_to_existing_payload(self) -> None: """ - ### Summary + # Summary + Add a file to the payload if the following are true: + - The serialNumber and partition associated with the file exist in the payload. - The file does not already exist in the files list for that serialNumber and partition. - ### Raises + ## Raises + None - ### Details + ## Details + We are looking at the following structure. ```json @@ -397,28 +443,36 @@ def add_file_to_existing_payload(self): }, ] } + ``` + """ + serial_number: str = self.ip_address_to_serial_number(self.ip_address) + for item in self.payload["deleteFiles"]: - serial_number = item.get("serialNumber") - partition = item.get("partition") - if serial_number != self.ip_address_to_serial_number(self.ip_address): + if item.get("serialNumber") != serial_number: continue - if partition != self.partition: + if item.get("partition") != self.partition: continue - files = item.get("files") - for file in files: - if file.get("fileName") == self.filename and file.get("bootflashType") == self.supervisor: - return - files.append( - { - "bootflashType": self.supervisor, - "fileName": self.filename, - "filePath": self.filepath, - } - ) - item.update({"files": files}) - def add_file_to_payload(self): + # Ensure files list exists + if "files" not in item: + item["files"] = [] + + # Check if file already exists (same filename AND supervisor) + for file in item["files"]: + if (file.get("fileName") == self.filename and + file.get("bootflashType") == self.supervisor): + return # File already in payload + + # Add the new file + item["files"].append({ + "bootflashType": self.supervisor, + "fileName": self.filename, + "filePath": self.filepath, + }) + return # Done - exit after finding matching item + + def add_file_to_payload(self) -> None: """ ### Summary Add a file to the payload if the serialNumber and partition do not @@ -443,7 +497,7 @@ def add_file_to_payload(self): else: self.add_file_to_existing_payload() - def add_file(self): + def add_file(self) -> None: """ ### Summary Add a file to the payload. @@ -452,7 +506,7 @@ def add_file(self): - ``ValueError`` if: - The switch does not allow file deletion. """ - method_name = inspect.stack()[0][3] + method_name: str = inspect.stack()[0][3] self.validate_prerequisites_for_add_file() if not self.ok_to_delete_files(self.ip_address): @@ -464,7 +518,7 @@ def add_file(self): self.add_file_to_payload() self.update_diff() - def update_diff(self): + def update_diff(self) -> None: """ ### Summary Update ``diff`` with ``target``. @@ -478,13 +532,13 @@ def update_diff(self): - ``target`` has already been validated to be a dictionary and to contain ``ip_address`` in ``target.setter``. """ - ip_address = self.target.get("ip_address") + ip_address: str = self.target.get("ip_address", "") if ip_address not in self.diff: self.diff[ip_address] = [] self.diff[ip_address].append(self.target) @property - def filepath(self): + def filepath(self) -> str: """ ### Summary Return the current ``filepath``. @@ -504,11 +558,11 @@ def filepath(self): return self._filepath @filepath.setter - def filepath(self, value): + def filepath(self, value: str) -> None: self._filepath = value @property - def filename(self): + def filename(self) -> str: """ ### Summary Return the current ``filename``. @@ -527,11 +581,11 @@ def filename(self): return self._filename @filename.setter - def filename(self, value): + def filename(self, value: str) -> None: self._filename = value @property - def ip_address(self): + def ip_address(self) -> str: """ ### Summary The ip address of the switch on which ``filename`` resides. @@ -548,11 +602,11 @@ def ip_address(self): return self._ip_address @ip_address.setter - def ip_address(self, value): + def ip_address(self, value: str) -> None: self._ip_address = value @property - def partition(self): + def partition(self) -> str: """ ### Summary The partition on which ``filename`` resides. @@ -569,11 +623,11 @@ def partition(self): return self._partition @partition.setter - def partition(self, value): + def partition(self, value: str) -> None: self._partition = value @property - def rest_send(self): + def rest_send(self) -> RestSend: """ # Summary @@ -655,7 +709,7 @@ def results(self, value: Results) -> None: self._results = value @property - def supervisor(self): + def supervisor(self) -> str: """ ### Summary Return the current ``supervisor``. @@ -676,11 +730,11 @@ def supervisor(self): return self._supervisor @supervisor.setter - def supervisor(self, value): + def supervisor(self, value: str) -> None: self._supervisor = value @property - def switch_details(self): + def switch_details(self) -> SwitchDetails: """ ### Summary An instance of the ``SwitchDetails()`` class. @@ -709,7 +763,7 @@ def switch_details(self, value): self._switch_details = value @property - def target(self): + def target(self) -> dict[str, str]: """ ### Summary ``target`` is a dictionary that is used to set the diff passed to @@ -759,7 +813,7 @@ def target(self): return self._target @target.setter - def target(self, value): + def target(self, value: dict[str, str]) -> None: method_name = inspect.stack()[0][3] msg = f"{self.class_name}.{method_name}: " diff --git a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py index 463d27118..73d5d7eb8 100644 --- a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py +++ b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_files.py @@ -75,18 +75,18 @@ def test_bootflash_files_00000() -> None: "serial_number", "supervisor", ] - assert instance.ok_to_delete_files_reason is None + assert instance.ok_to_delete_files_reason == "" assert instance.payload == {"deleteFiles": []} - assert instance.filename is None - assert instance.filepath is None - assert instance.ip_address is None - assert instance.partition is None + assert instance._filename == "" + assert instance._filepath == "" + assert instance._ip_address == "" + assert instance._partition == "" assert instance._rest_send.params == {} assert instance._rest_send.class_name == "RestSend" assert instance._results.class_name == "Results" - assert instance.supervisor is None - assert instance.switch_details is None - assert instance.target is None + assert instance._supervisor == "" + assert instance._switch_details.class_name == "SwitchDetails" + assert instance._target == {} def test_bootflash_files_00100() -> None: @@ -142,30 +142,31 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() convert_target = ConvertTargetToParams() convert_target.target = gen_targets.next convert_target.commit() - instance.filepath = convert_target.filepath - instance.filename = convert_target.filename + instance.filepath = convert_target.filepath or "" + instance.filename = convert_target.filename or "" instance.ip_address = "172.22.150.112" - instance.partition = convert_target.partition - instance.supervisor = convert_target.supervisor - instance.target = convert_target.target + instance.partition = convert_target.partition or "" + instance.supervisor = convert_target.supervisor or "" + instance.target = convert_target.target or {} instance.add_file() convert_target = ConvertTargetToParams() convert_target.target = gen_targets.next convert_target.commit() - instance.filepath = convert_target.filepath - instance.filename = convert_target.filename + instance.filepath = convert_target.filepath or "" + instance.filename = convert_target.filename or "" instance.ip_address = "172.22.150.113" - instance.partition = convert_target.partition - instance.supervisor = convert_target.supervisor - instance.target = convert_target.target + instance.partition = convert_target.partition or "" + instance.supervisor = convert_target.supervisor or "" + instance.target = convert_target.target or {} instance.add_file() instance.commit() @@ -195,7 +196,8 @@ def test_bootflash_files_00110() -> None: instance = BootflashFiles() instance.switch_details = SwitchDetails() - match = r"BootflashFiles\.rest_send: RestSend.params must be set before accessing\." + match = r"BootflashFiles\.validate_commit_parameters: " + match += r"rest_send must be set before calling commit\(\)\." with pytest.raises(ValueError, match=match): instance.commit() @@ -205,11 +207,10 @@ def test_bootflash_files_00130() -> None: ### Classes and Methods - BootflashFiles() - commit() - - validate_commit_parameters() + - switch_details property ### Summary - Verify ``ValueError`` is raised if ``switch_details`` is not set before - calling commit. + Verify `ValueError` is raised if `switch_details` is not set before calling commit. ### Test - ValueError is raised by validate_commit_parameters(). @@ -220,7 +221,7 @@ def test_bootflash_files_00130() -> None: instance.rest_send = RestSend(params_deleted) instance.results = Results() - match = r"BootflashFiles.validate_commit_parameters:\s+" + match = r"BootflashFiles\.validate_commit_parameters: " match += r"switch_details must be set before calling commit\(\)\." with pytest.raises(ValueError, match=match): instance.commit() @@ -268,6 +269,7 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() instance.filepath = "bootflash:/air.txt" @@ -335,6 +337,7 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() instance.filepath = "bootflash:/air.txt" @@ -427,18 +430,19 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() convert_target = ConvertTargetToParams() convert_target.target = gen_targets.next convert_target.commit() - instance.filepath = convert_target.filepath - instance.filename = convert_target.filename + instance.filepath = convert_target.filepath or "" + instance.filename = convert_target.filename or "" instance.ip_address = "172.22.150.112" - instance.partition = convert_target.partition - instance.supervisor = convert_target.supervisor - instance.target = convert_target.target + instance.partition = convert_target.partition or "" + instance.supervisor = convert_target.supervisor or "" + instance.target = convert_target.target or {} match = r"BootflashFiles\.add_file:\s+" match += r"Cannot delete files on switch 172\.22\.150\.112\.\s+" @@ -632,6 +636,12 @@ def test_bootflash_files_00280() -> None: - ValueError is raised by validate_prerequisites_for_add_file(). - Error message matches expectation. """ + rest_send = RestSend(params_deleted) + rest_send.unit_test = True + rest_send.timeout = 1 + rest_send.response_handler = ResponseHandler() + rest_send.sender = Sender() + with does_not_raise(): instance = BootflashFiles() instance.filename = "air.txt" @@ -641,6 +651,8 @@ def test_bootflash_files_00280() -> None: instance.results = Results() instance.supervisor = "active" instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send + instance.switch_details.results = Results() match = r"BootflashFiles.validate_prerequisites_for_add_file:\s+" match += r"target must be set before calling add_file\(\)\." @@ -695,7 +707,8 @@ def test_bootflash_files_00310() -> None: instance = BootflashFiles() instance.switch_details = SwitchDetails() - match = r"BootflashFiles\.rest_send: RestSend.params must be set before accessing\." + match = r"BootflashFiles.refresh_switch_details: " + match += r"rest_send must be set before calling refresh_switch_details\." with pytest.raises(ValueError, match=match): instance.refresh_switch_details() @@ -747,6 +760,7 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() match = r"BootflashFiles\.ip_address_to_serial_number:\s+" @@ -789,6 +803,7 @@ def test_bootflash_files_00500() -> None: instance.switch_details.results = Results() instance.delete_files() + assert instance.results.response_current is not None assert instance.results.response_current["RETURN_CODE"] == 200 assert instance.results.response_current["MESSAGE"] == "No files to delete." assert instance.results.result == [{"success": True, "changed": False, "sequence_number": 1}] @@ -837,6 +852,7 @@ def responses(): instance.results = Results() instance.switch_details = SwitchDetails() instance.switch_details.results = Results() + instance.switch_details.rest_send = rest_send instance.payload = payloads_bootflash_files(key) instance.ip_address = "172.22.150.112" instance.partition = "usb1:" @@ -887,6 +903,7 @@ def responses(): instance.rest_send = rest_send instance.results = Results() instance.switch_details = SwitchDetails() + instance.switch_details.rest_send = rest_send instance.switch_details.results = Results() instance.payload = payloads_bootflash_files(key) instance.ip_address = "172.22.150.112" @@ -964,7 +981,7 @@ def test_bootflash_files_00800() -> None: match = r"BootflashFiles.target:\s+" match += r"target must be a dictionary\. Got type str for value foo\." with pytest.raises(TypeError, match=match): - instance.target = "foo" + instance.target = "foo" # type: ignore @pytest.mark.parametrize("parameter", ["filepath", "ip_address", "serial_number", "supervisor"]) From a6e92d7228a02269bfe5e1679242f4d68fa9f87c Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Fri, 14 Nov 2025 14:47:03 -1000 Subject: [PATCH 6/7] Appease black and sanity linters Fix the below sanity error by running through the black linter: ERROR: plugins/module_utils/bootflash/bootflash_files.py:464:5: E129: visually indented line with same indent as next logical line --- plugins/module_utils/bootflash/bootflash_files.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/plugins/module_utils/bootflash/bootflash_files.py b/plugins/module_utils/bootflash/bootflash_files.py index 548d8302c..31c279866 100644 --- a/plugins/module_utils/bootflash/bootflash_files.py +++ b/plugins/module_utils/bootflash/bootflash_files.py @@ -460,16 +460,17 @@ def add_file_to_existing_payload(self) -> None: # Check if file already exists (same filename AND supervisor) for file in item["files"]: - if (file.get("fileName") == self.filename and - file.get("bootflashType") == self.supervisor): + if file.get("fileName") == self.filename and file.get("bootflashType") == self.supervisor: return # File already in payload # Add the new file - item["files"].append({ - "bootflashType": self.supervisor, - "fileName": self.filename, - "filePath": self.filepath, - }) + item["files"].append( + { + "bootflashType": self.supervisor, + "fileName": self.filename, + "filePath": self.filepath, + } + ) return # Done - exit after finding matching item def add_file_to_payload(self) -> None: From cb949cde6199c3297d18b8d5733eb6b11c197692 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 17 Nov 2025 09:57:34 -1000 Subject: [PATCH 7/7] RestSend must be passed to SwitchDetails 1. Recent changes to SwitchDetails exposed weakness in the unit tests in that the tests would pass without having passed an instance of RestSend to SwitchDetails. This commit updates the following two unit tests to pass RestSend to SwitchDetails. test_bootflash_deleted_03200 test_bootflash_deleted_03210 2. DcnmBootflash.Deleted - Need to pass RestSend instance to BootflashInfo.SwitchDetails - Need to pass Results instance to BootflashInfo.SwitchDetails - Need to pass RestSend instance to BootflashFiles.SwitchDetails --- plugins/modules/dcnm_bootflash.py | 3 +++ .../unit/modules/dcnm/dcnm_bootflash/test_bootflash_deleted.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/plugins/modules/dcnm_bootflash.py b/plugins/modules/dcnm_bootflash.py index 317670a79..4b7fc4e30 100644 --- a/plugins/modules/dcnm_bootflash.py +++ b/plugins/modules/dcnm_bootflash.py @@ -534,6 +534,8 @@ def commit(self) -> None: self.bootflash_info.results = Results() self.bootflash_info.rest_send = self.rest_send # pylint: disable=no-member self.bootflash_info.switch_details = SwitchDetails() + self.bootflash_info.switch_details.rest_send = self.rest_send + self.bootflash_info.switch_details.results = Results() # Retrieve bootflash contents for the user's switches. switch_list = [] @@ -549,6 +551,7 @@ def commit(self) -> None: self.bootflash_files.rest_send = self.rest_send self.bootflash_files.switch_details = SwitchDetails() self.bootflash_files.switch_details.results = Results() + self.bootflash_files.switch_details.rest_send = self.rest_send # Update BootflashFiles() with the files to delete self.files_to_delete = {} diff --git a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_deleted.py b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_deleted.py index 5d4c6e25b..9b460de9c 100644 --- a/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_deleted.py +++ b/tests/unit/modules/dcnm/dcnm_bootflash/test_bootflash_deleted.py @@ -337,7 +337,9 @@ def responses(): instance.rest_send = rest_send instance.bootflash_files.switch_details = SwitchDetails() instance.bootflash_files.rest_send = rest_send + instance.bootflash_files.switch_details.rest_send = rest_send instance.bootflash_files.switch_details.results = Results() + match = r"Deleted\.update_bootflash_files:\s+" match += r"Error adding file to bootflash_files\.\s+" match += r"Error detail:\s+" @@ -401,6 +403,7 @@ def responses(): instance.rest_send = rest_send instance.bootflash_files.switch_details = SwitchDetails() instance.bootflash_files.rest_send = rest_send + instance.bootflash_files.switch_details.rest_send = rest_send instance.bootflash_files.switch_details.results = Results() match = r"Deleted\.update_bootflash_files:\s+" match += r"Error adding file to bootflash_files\.\s+"