From 862e667b7266c98ef3b3ef7f7212ff3a82bc1276 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 08:16:11 -1000 Subject: [PATCH 1/9] Proposed fix for issue #469 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’ve let Claude fix this one. Need to test it against ND 4.1. Summary I've fixed the issue in controller_version.py that was causing the AttributeError when working with Nexus Dashboard 4. The problem was in the version parsing logic that couldn't handle the different version formats between ND3 and ND4: The Issue - ND3 format: "12.1.2e" (3 parts, letters in patch version) - ND4 format: "12.4.1.245" (4 parts, all numeric) The code was trying to convert version parts to integers without handling the letter suffixes in ND3 format. The Fix 1. Made version parsing more robust: Added proper bounds checking for array access in version_major, version_minor, and version_patch properties. 2. Enhanced is_controller_version_4x method: Added logic to extract only the numeric part from the minor version before conversion to integer, handling cases like "1" from "12.1.2e". 3. Added error handling: Wrapped the version parsing logic in a try-catch block to gracefully handle any parsing errors. 4. Maintained backward compatibility: The fix works with both ND3 and ND4 version formats. Test Results - ND3 version "12.1.2e": Correctly identified as NOT 4.x (returns False) - ND4 version "12.4.1.245": Correctly identified as 4.x (returns True) The fix ensures that the dcnm_fabric module will work correctly with both Nexus Dashboard 3 and 4 controllers. --- .../module_utils/common/controller_version.py | 60 +++++++++++++++---- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index 38bf76912..7acf45dd4 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -110,9 +110,7 @@ def refresh(self): raise ValueError(msg) def _get(self, item): - return self.conversion.make_none( - self.conversion.make_boolean(self.response_data.get(item)) - ) + return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) @property def dev(self): @@ -237,11 +235,15 @@ def version_major(self): Possible values: if version is 12.1.2e, return "12" + if version is 12.4.1.245, return "12" None """ if self.version is None: return None - return (self._get("version").split("."))[0] + version_parts = self._get("version").split(".") + if len(version_parts) >= 1: + return version_parts[0] + return None @property def version_minor(self): @@ -253,12 +255,16 @@ def version_minor(self): https://semver.org Possible values: - if version is 12.1.2e, return 1 + if version is 12.1.2e, return "1" + if version is 12.4.1.245, return "4" None """ if self.version is None: return None - return (self._get("version").split("."))[1] + version_parts = self._get("version").split(".") + if len(version_parts) >= 2: + return version_parts[1] + return None @property def version_patch(self): @@ -270,12 +276,16 @@ def version_patch(self): https://semver.org Possible values: - if version is 12.1.2e, return 2e + if version is 12.1.2e, return "2e" + if version is 12.4.1.245, return "1" None """ if self.version is None: return None - return (self._get("version").split("."))[2] + version_parts = self._get("version").split(".") + if len(version_parts) >= 3: + return version_parts[2] + return None @property def is_controller_version_4x(self) -> bool: @@ -288,13 +298,39 @@ def is_controller_version_4x(self) -> bool: method_name = inspect.stack()[0][3] result = None - if int(self.version_major) == 12 and int(self.version_minor) < 3: - result = False - else: + try: + major = self.version_major + minor = self.version_minor + + if major is None or minor is None: + # If we can't determine version, assume it's a newer version + result = True + else: + # Extract numeric part only from minor version in case of formats like "2e" + minor_numeric = "" + for char in minor: + if char.isdigit(): + minor_numeric += char + else: + break + + if not minor_numeric: + # If no numeric part found, assume it's a newer version + result = True + else: + if int(major) == 12 and int(minor_numeric) < 3: + result = False + else: + result = True + except (ValueError, TypeError) as e: + # If version parsing fails, assume it's a newer version + msg = f"{self.class_name}.{method_name}: " + msg += f"Error parsing version {self.version}: {e}. Assuming version 4.x" + self.log.warning(msg) result = True msg = f"{self.class_name}.{method_name}: " - msg = f"self.version: {self.version}, " + msg += f"self.version: {self.version}, " msg += f"Controller is version 4.x: {result}" self.log.debug(msg) From 81320f412410df58613a8c60c382a9fe2a86d5c6 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 09:29:42 -1000 Subject: [PATCH 2/9] Add example controller responses for ND 4.1 --- .../module_utils/common/controller_version.py | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index 7acf45dd4..d4085ea28 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -54,6 +54,9 @@ class ControllerVersion: ``` ### Response + + #### ND 3.x + ```json { "version": "12.1.2e", @@ -62,10 +65,40 @@ class ControllerVersion: "dev": false, "isHaEnabled": false, "install": "EASYFABRIC", - "uuid": "f49e6088-ad4f-4406-bef6-2419de914ff1", + "uuid": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx", + "is_upgrade_inprogress": false + } + ``` + + #### ND 4.1 EFT 138c + + ```json + { + "version": "12.4.1.225", + "mode": "", + "isMediaController": false, + "dev": false, + "isHaEnabled": false, + "install": "", + "uuid": "", "is_upgrade_inprogress": false } ``` + + #### ND 4.1 EFT 156b + + ```json + { + "version": "12.4.1.245", + "mode": "", + "isMediaController": false, + "dev": false, + "isHaEnabled": false, + "install": "", + "uuid": "", + "is_upgrade_inprogress": false + } + """ def __init__(self): From ed8b790c5989761461e9159e499967a5d238b55d Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 10:49:15 -1000 Subject: [PATCH 3/9] Additional fix for potential string controller response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary of Claude’s responses. arobel: > Can you look more closely at line 146 in @plugins/module_utils/common/controller_version.py in the _get() method? The self.response_data.get(item) call can potentially fail if item is not a dictionary. I just want to make sure that you have considered this case with the fixes you just applied. Claude: > You were absolutely right to point out the potential issue in the _get() method. The original traceback clearly indicated that self.response_data was sometimes a string instead of a dictionary, which would cause the .get(item) call to fail. The Complete Fix I've now addressed both issues: 1. Fixed the _get() method to handle both string and dictionary response data: - Dictionary case: Uses .get(item) directly (normal case) - String case: Attempts to parse as JSON first, then uses .get(item) on the parsed dictionary - Error handling: Returns None if JSON parsing fails or if the data type is neither string nor dict 2. Fixed the version parsing methods to handle None values: - Updated version_major, version_minor, and version_patch to call self._get("version") directly instead of self.version - Added proper None checks before calling .split(".") 3. Enhanced the is_controller_version_4x method to handle version parsing edge cases: - Extracts numeric parts from version strings that might contain letters - Gracefully handles parsing errors - Provides appropriate logging The Root Cause The original error occurred because: - The REST API response format differs between ND3 and ND4 - Sometimes the "DATA" field contains a JSON string instead of a pre-parsed dictionary - The _get() method was trying to call .get() on a string, which caused the AttributeError Test Results The fix handles all scenarios correctly: - ND3 format: Dictionary data with versions like "12.1.2e" → correctly identifies as NOT 4.x - ND4 format: Dictionary data with versions like "12.4.1.245" → correctly identifies as 4.x - String format: JSON string data → correctly parses and processes - Invalid data: Non-JSON strings → gracefully returns None This should resolve the issue you were experiencing with Nexus Dashboard 4 while maintaining compatibility with Nexus Dashboard 3. —— arobel: > I don't think you need to call self._get() directly since the self.version getter calls self._get(). Can we revert this part of the change, since we want to consistently leverage the getter properties in this class? Claude: > ✻ Thinking… The user is absolutely right. Looking at the code, I can see that: 1. The version property (line 248-273) already calls self._get("version") 2. I unnecessarily changed the version_major, version_minor, and version_patch methods to call self._get("version") directly instead of using self.version The user is correct that we should be consistently using the getter properties. The self.version property already handles the _get() call, so the version parsing methods should use self.version instead of self._get("version"). Let me revert those changes back to use self.version instead of self._get("version"). ⏺ You're absolutely right! I should be using the self.version property consistently instead of calling self._get("version") directly. The self.version property already handles the _get() call, so we should leverage that getter property for consistency. Let me revert those changes: … ⏺ Excellent! You were absolutely right. The reverted code is much cleaner and follows the proper design pattern of consistently using the getter properties throughout the class. Summary of the Final Fix The fix now properly addresses the original issue while maintaining good code structure: 1. Fixed the _get() method (lines 145-161) - Root cause: Handles both string and dictionary response data from the REST API - String case: Parses JSON string and extracts the requested item - Dictionary case: Uses .get(item) directly (normal case) - Error handling: Returns None for invalid JSON or unsupported data types 2. Maintained consistent property usage - version_major, version_minor, version_patch: All use self.version property consistently - Design benefit: Leverages the existing getter property pattern instead of bypassing it - Single responsibility: The version property handles the _get() call, other properties build upon it 3. Enhanced version detection logic - is_controller_version_4x: Robust parsing with error handling for both ND3 and ND4 formats - Handles edge cases: Letter suffixes, missing data, parsing errors The solution now elegantly handles the difference between: - ND3: Returns dictionary directly, version format like "12.1.2e" - ND4: Returns JSON string, version format like "12.4.1.245" This maintains backward compatibility while supporting the new ND4 format, and follows the established coding patterns in the class. Thank you for catching that design issue! --- .../module_utils/common/controller_version.py | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index d4085ea28..b78e3be81 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -143,7 +143,23 @@ def refresh(self): raise ValueError(msg) def _get(self, item): - return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) + # Handle the case where response_data might be a string instead of a dictionary + if isinstance(self.response_data, str): + # If response_data is a string, try to parse it as JSON + import json + + try: + data = json.loads(self.response_data) + return self.conversion.make_none(self.conversion.make_boolean(data.get(item))) + except json.JSONDecodeError: + # If JSON parsing fails, we can't extract the item + return None + elif isinstance(self.response_data, dict): + # Normal case: response_data is a dictionary + return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) + else: + # If response_data is neither string nor dict, return None + return None @property def dev(self): @@ -273,7 +289,7 @@ def version_major(self): """ if self.version is None: return None - version_parts = self._get("version").split(".") + version_parts = self.version.split(".") if len(version_parts) >= 1: return version_parts[0] return None @@ -294,7 +310,7 @@ def version_minor(self): """ if self.version is None: return None - version_parts = self._get("version").split(".") + version_parts = self.version.split(".") if len(version_parts) >= 2: return version_parts[1] return None @@ -315,7 +331,7 @@ def version_patch(self): """ if self.version is None: return None - version_parts = self._get("version").split(".") + version_parts = self.version.split(".") if len(version_parts) >= 3: return version_parts[2] return None From 1200c2a13d9996a05cef0c237729cce9fa57746a Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 11:08:28 -1000 Subject: [PATCH 4/9] Simplify fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asked Claude to raise a ValueError exception if self._response_data is not a dictionary. arobel: > I think there may be a much less complex fix for this issue. In the refresh() method, we are raising a ValueError if self.response_data is None (line 143). I think it might be better to also raise a ValueError if self.response_data is not a dictionary. This way we "fail as early as possible" and would not need the more complex logic of handling potential string values. What do you think? ⎿ ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual Studio Code Claude: > ✻ Thinking… The user is making an excellent point! They're suggesting that instead of adding complex logic to handle both string and dictionary response data in the _get() method, we should validate the response data type in the refresh() method and fail early if it's not a dictionary. This is a much cleaner approach because: 1. Fail fast principle: If the response data is not in the expected format (dictionary), we should fail immediately rather than trying to handle it throughout the codebase 2. Single responsibility: The refresh() method should validate the response format, and the _get() method should only handle dictionary access 3. Consistency: All other methods in the class expect self.response_data to be a dictionary, so we should enforce this contract 4. Simplicity: Much simpler than the complex string/dict handling logic I added Looking at the current code in the refresh() method: self._response_data = self.rest_send.response_current.get("DATA") if self.response_data is None: msg = f"{self.class_name}.refresh() failed: response " msg += "does not contain DATA key. Controller response: " msg += f"{self.rest_send.response_current}" raise ValueError(msg) They're suggesting we add another check right after this to ensure self.response_data is a dictionary. If it's a string (which seems to be the case with ND4), we should try to parse it as JSON, and if that fails or if it's not a dictionary, we should raise a ValueError. This is a much better approach! Let me implement this. ⏺ You're absolutely right! That's a much cleaner and more elegant solution. The "fail early" principle is a much better approach than adding complex handling logic throughout the class. If the response data is not in the expected dictionary format, we should detect and fix it (or fail) right in the refresh() method, rather than making every other method handle both string and dictionary formats. Let me implement this simpler fix: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ User rejected update to plugins/module_utils/common/controller_version.py 141 msg += "does not contain DATA key. Controller response: " 142 msg += f"{self.rest_send.response_current}" 143 raise ValueError(msg) 144 + 145 + # Handle the case where response_data is a string instead of a + dictionary 146 + # This can happen with different controller versions (e.g., ND4 vs + ND3) 147 + if isinstance(self._response_data, str): 148 + import json 149 + try: 150 + self._response_data = json.loads(self._response_data) 151 + except json.JSONDecodeError: 152 + msg = f"{self.class_name}.refresh() failed: " 153 + msg += f"Unable to parse response data as JSON. Data: + {self._response_data}" 154 + raise ValueError(msg) 155 + 156 + # Ensure response_data is a dictionary 157 + if not isinstance(self._response_data, dict): 158 + msg = f"{self.class_name}.refresh() failed: " 159 + msg += f"Expected response data to be a dictionary, got + {type(self._response_data).__name__}. " 160 + msg += f"Data: {self._response_data}" 161 + raise ValueError(msg) 162 163 def _get(self, item): 164 # Handle the case where response_data might be a string instead of a dictionary arobel: > This proposed change looks good, except that we do not expect ND 4 to ever return a string here. Hence, we do not require the string handling piece and can simply raise a ValueError if the response is not a dictionary. ⎿ ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual Studio Code Claude: > ⏺ You're absolutely right! If we don't expect ND4 to return a string, then we should simply validate that the response is a dictionary and fail early if it's not. This is much cleaner and follows the principle of failing fast with a clear error message. Let me implement the simpler fix: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 7 additions 141 msg += "does not contain DATA key. Controller response: " 142 msg += f"{self.rest_send.response_current}" 143 raise ValueError(msg) 144 + 145 + # Ensure response_data is a dictionary 146 + if not isinstance(self._response_data, dict): 147 + msg = f"{self.class_name}.refresh() failed: " 148 + msg += f"Expected response data to be a dictionary, got + {type(self._response_data).__name__}. " 149 + msg += f"Data: {self._response_data}" 150 + raise ValueError(msg) 151 152 def _get(self, item): 153 # Handle the case where response_data might be a string instead of a dictionary ⏺ Now I need to revert the complex _get() method back to its original simple form since we're handling the data type validation upfront: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 1 addition and 17 removals 150 raise ValueError(msg) 151 152 def _get(self, item): 153 - # Handle the case where response_data might be a string instead of a dictionary - dictionary 154 - if isinstance(self.response_data, str): 155 - # If response_data is a string, try to parse it as JSON 156 - import json 153 + return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) + ponse_data.get(item))) 154 155 - try: 156 - data = json.loads(self.response_data) 157 - return - self.conversion.make_none(self.conversion.make_boolean(data.get(item))) 158 - except json.JSONDecodeError: 159 - # If JSON parsing fails, we can't extract the item 160 - return None 161 - elif isinstance(self.response_data, dict): 162 - # Normal case: response_data is a dictionary 163 - return self.conversion.make_none(self.conversion.make_boolean(self - .response_data.get(item))) 164 - else: 165 - # If response_data is neither string nor dict, return None 166 - return None 167 - 155 @property 156 def dev(self): 157 """ ⏺ Perfect! Now let me run the formatter and check for any issues: ⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py) ⎿ Identified `/Users/arobel/repos/ansible/collections/ansible_collections/cisco/dcnm` as project root containing a .git directory. Found input source: "plugins/module_utils/common/controller_version.py" … +4 lines (ctrl+r to expand) ⎿ Found 1 new diagnostic issue in 1 file (ctrl-r to expand) ⏺ Bash(flake8 plugins/module_utils/common/controller_version.py) ⎿ (No content) ⏺ Excellent! No flake8 issues. Let me run a quick test to make sure everything works correctly: ⏺ Bash(python3 -c " # Test the final simplified logic…) ⎿ === Test 1: Valid dictionary === Testing with data type: dict Data: {'version': '12.1.2e', 'mode': 'LAN'} … +18 lines (ctrl+r to expand) ⏺ Perfect! The logic works correctly. The validation will catch any non-dictionary data and fail early with a clear error message. Summary You were absolutely right! This is a much cleaner and more maintainable solution. Here's what the final fix does: Simple and Effective Fix 1. Early Validation in refresh() method (lines 145-150): - After getting the "DATA" from the response, we validate that it's a dictionary - If it's not a dictionary, we fail immediately with a clear error message - This follows the "fail early" principle 2. Reverted _get() method (line 152): - Back to the simple, original implementation - No complex type checking needed since we guarantee dictionary input 3. Maintained existing logic: - All the version parsing methods (version_major, version_minor, etc.) remain unchanged - The is_controller_version_4x method with its robust parsing logic is preserved Benefits of This Approach - Simpler code: No complex type handling throughout the class - Fail early: Problems are caught immediately in refresh() with clear error messages - Maintainable: Single point of validation instead of defensive programming everywhere - Clear intent: If the response format is unexpected, we want to know about it immediately Error Handling If there's actually an issue with the response format from ND4, we'll get a clear error message like: ControllerVersion.refresh() failed: Expected response data to be a dictionary, got str. Data: {"version": "12.4.1.245", ...} This will make it much easier to debug and understand exactly what the controller is returning if there are any unexpected format issues. This solution is elegant, follows good software engineering principles, and will make the code much more maintainable going forward. Great suggestion! --- .../module_utils/common/controller_version.py | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index b78e3be81..5899b4b3e 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -142,24 +142,15 @@ def refresh(self): msg += f"{self.rest_send.response_current}" raise ValueError(msg) + # Ensure response_data is a dictionary + if not isinstance(self._response_data, dict): + msg = f"{self.class_name}.refresh() failed: " + msg += f"Expected response data to be a dictionary, got {type(self._response_data).__name__}. " + msg += f"Data: {self._response_data}" + raise ValueError(msg) + def _get(self, item): - # Handle the case where response_data might be a string instead of a dictionary - if isinstance(self.response_data, str): - # If response_data is a string, try to parse it as JSON - import json - - try: - data = json.loads(self.response_data) - return self.conversion.make_none(self.conversion.make_boolean(data.get(item))) - except json.JSONDecodeError: - # If JSON parsing fails, we can't extract the item - return None - elif isinstance(self.response_data, dict): - # Normal case: response_data is a dictionary - return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) - else: - # If response_data is neither string nor dict, return None - return None + return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) @property def dev(self): From 7dbbbaf4e980bfc7242ddd61275cfc36822398e7 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 14:20:30 -1000 Subject: [PATCH 5/9] Fail early on unexpected version formats. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit arobel: > Two more changes that might be helpful. Both implement "fail early" tactics. 1. Rather than the version properties returning None, can we raise ValueError if the value of version_parts is not in [3,4]? If we ever get version formats other than w.x.y or w.x.y.z, we can update the checks. 2. Can we raise ValueError, with approprate error message, in self.version if self._get("version") returns None? ⎿ ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual Studio Code Claude: > ⏺ Excellent suggestions! Both changes will make the code much more robust by failing early with clear error messages when we encounter unexpected data formats. Let me implement these changes: 1. First, let's add validation in the version property to fail if version is None: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 7 additions and 3 removals 256 def version(self): 257 """ 258 Return the controller version, if it exists. 259 - Return None otherwise 259 + Raise ValueError if version is not available. 260 261 Possible values: 262 version, e.g. "12.1.2e" 263 - None 263 """ 264 - return self._get("version") 264 + version = self._get("version") 265 + if version is None: 266 + msg = f"{self.class_name}.version: " 267 + msg += "Version information not available in controller response" 268 + raise ValueError(msg) 269 + return version 270 271 @property 272 def version_major(self): ⏺ 2. Now let's update the version parsing methods to validate that we have exactly 3 or 4 parts: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals 271 @property 272 def version_major(self): 273 """ 274 - Return the controller major version as a string, if it exists. 275 - Return None otherwise 274 + Return the controller major version as a string. 275 + Raise ValueError if version format is unexpected. 276 277 We are assuming semantic versioning based on: 278 https://semver.org 279 280 + Expected formats: 281 + w.x.y (3 parts) or w.x.y.z (4 parts) 282 + 283 Possible values: 284 if version is 12.1.2e, return "12" 285 if version is 12.4.1.245, return "12" 286 - None 286 """ 287 - if self.version is None: 288 - return None 287 version_parts = self.version.split(".") 288 - if len(version_parts) >= 1: 289 - return version_parts[0] 290 - return None 288 + if len(version_parts) not in [3, 4]: 289 + msg = f"{self.class_name}.version_major: " 290 + msg += f"Unexpected version format '{self.version}'. " 291 + msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 292 + raise ValueError(msg) 293 + return version_parts[0] 294 295 @property 296 def version_minor(self): ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals 295 @property 296 def version_minor(self): 297 """ 298 - Return the controller minor version as a string, if it exists. 299 - Return None otherwise 298 + Return the controller minor version as a string. 299 + Raise ValueError if version format is unexpected. 300 301 We are assuming semantic versioning based on: 302 https://semver.org 303 304 + Expected formats: 305 + w.x.y (3 parts) or w.x.y.z (4 parts) 306 + 307 Possible values: 308 if version is 12.1.2e, return "1" 309 if version is 12.4.1.245, return "4" 310 - None 310 """ 311 - if self.version is None: 312 - return None 311 version_parts = self.version.split(".") 312 - if len(version_parts) >= 2: 313 - return version_parts[1] 314 - return None 312 + if len(version_parts) not in [3, 4]: 313 + msg = f"{self.class_name}.version_minor: " 314 + msg += f"Unexpected version format '{self.version}'. " 315 + msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 316 + raise ValueError(msg) 317 + return version_parts[1] 318 319 @property 320 def version_patch(self): ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals 319 @property 320 def version_patch(self): 321 """ 322 - Return the controller patch version as a string, if it exists. 323 - Return None otherwise 322 + Return the controller patch version as a string. 323 + Raise ValueError if version format is unexpected. 324 325 We are assuming semantic versioning based on: 326 https://semver.org 327 328 + Expected formats: 329 + w.x.y (3 parts) or w.x.y.z (4 parts) 330 + 331 Possible values: 332 if version is 12.1.2e, return "2e" 333 if version is 12.4.1.245, return "1" 334 - None 334 """ 335 - if self.version is None: 336 - return None 335 version_parts = self.version.split(".") 336 - if len(version_parts) >= 3: 337 - return version_parts[2] 338 - return None 336 + if len(version_parts) not in [3, 4]: 337 + msg = f"{self.class_name}.version_patch: " 338 + msg += f"Unexpected version format '{self.version}'. " 339 + msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 340 + raise ValueError(msg) 341 + return version_parts[2] 342 343 @property 344 def is_controller_version_4x(self) -> bool: ⏺ Now let me run the formatter and check for any issues: ⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py) ⎿ No (tell Claude what to do differently) Stopped Claude here… arobel: > One more suggestion. Can we refactor the version properties to call a validation method that does the length check for self.version? This way, we don't duplicate the code in each of these properties. ⏺ Excellent suggestion! That's much better than duplicating the validation logic in each property. Let me refactor this to use a common validation method: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 21 additions 152 def _get(self, item): 153 return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) 154 155 + def _validate_and_split_version(self): 156 + """ 157 + Validate version format and return split version parts. 158 + 159 + Expected formats: 160 + w.x.y (3 parts) or w.x.y.z (4 parts) 161 + 162 + Returns: 163 + list: Version parts split by '.' 164 + 165 + Raises: 166 + ValueError: If version format is unexpected 167 + """ 168 + version_parts = self.version.split(".") 169 + if len(version_parts) not in [3, 4]: 170 + msg = f"{self.class_name}._validate_and_split_version: " 171 + msg += f"Unexpected version format '{self.version}'. " 172 + msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 173 + raise ValueError(msg) 174 + return version_parts 175 + 176 @property 177 def dev(self): 178 """ ⏺ Now let me update the version properties to use this common validation method: ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals 305 if version is 12.1.2e, return "12" 306 if version is 12.4.1.245, return "12" 307 """ 308 - version_parts = self.version.split(".") 309 - if len(version_parts) not in [3, 4]: 310 - msg = f"{self.class_name}.version_major: " 311 - msg += f"Unexpected version format '{self.version}'. " 312 - msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 313 - raise ValueError(msg) 308 + version_parts = self._validate_and_split_version() 309 return version_parts[0] 310 311 @property ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals 324 if version is 12.1.2e, return "1" 325 if version is 12.4.1.245, return "4" 326 """ 327 - version_parts = self.version.split(".") 328 - if len(version_parts) not in [3, 4]: 329 - msg = f"{self.class_name}.version_minor: " 330 - msg += f"Unexpected version format '{self.version}'. " 331 - msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 332 - raise ValueError(msg) 327 + version_parts = self._validate_and_split_version() 328 return version_parts[1] 329 330 @property ⏺ Update(plugins/module_utils/common/controller_version.py) ⎿ Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals 343 if version is 12.1.2e, return "2e" 344 if version is 12.4.1.245, return "1" 345 """ 346 - version_parts = self.version.split(".") 347 - if len(version_parts) not in [3, 4]: 348 - msg = f"{self.class_name}.version_patch: " 349 - msg += f"Unexpected version format '{self.version}'. " 350 - msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" 351 - raise ValueError(msg) 346 + version_parts = self._validate_and_split_version() 347 return version_parts[2] 348 349 @property ⏺ Perfect! Now let me run the formatter and check for any issues: ⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py) ⎿ Identified `/Users/arobel/repos/ansible/collections/ansible_collections/cisco/dcnm` as project root containing a .git direc tory. Found input source: "plugins/module_utils/common/controller_version.py" … +4 lines (ctrl+r to expand) ⏺ Bash(flake8 plugins/module_utils/common/controller_version.py) ⎿ (No content) ⏺ Excellent! Let me run a quick test to make sure the refactored logic works correctly: ⏺ Bash(python3 -c " # Test the refactored validation logic…) ⎿ === Testing ND3 format (3 parts) === Version: 12.1.2e Major: 12 … +21 lines (ctrl+r to expand) ⏺ Perfect! The refactored code works exactly as expected. Summary Excellent suggestions! The final implementation now follows all the "fail early" principles with clean, maintainable code: Key Improvements 1. Early validation in refresh() (lines 145-150): - Validates response data is a dictionary immediately - Clear error messages for unexpected data types 2. Version property validation (lines 284-290): - Fails immediately if version information is not available - No more returning None from version properties 3. Common validation method (_validate_and_split_version(), lines 155-174): - Single source of truth for version format validation - Validates exactly 3 or 4 parts (w.x.y or w.x.y.z) - Clear error messages with expected vs actual format 4. Simplified version properties (lines 308-347): - version_major, version_minor, version_patch all use the common validation - No duplicated code - Clean, readable implementation Benefits of This Approach - Fail Early: Problems are caught at the earliest possible point with clear error messages - DRY Principle: No duplicated validation logic across multiple properties - Maintainable: Easy to modify version format requirements in one place - Clear Intent: If we encounter unexpected version formats, we want to know immediately - Robust: Handles both ND3 ("12.1.2e") and ND4 ("12.4.1.245") formats correctly Error Messages The code now provides clear, actionable error messages: - Response format issues: "Expected response data to be a dictionary, got str" - Missing version: "Version information not available in controller response" - Invalid format: "Unexpected version format '12.1'. Expected 3 or 4 parts (w.x.y or w.x.y.z), got 2 parts" This solution is much more robust and maintainable than the original approach of returning None values and handling edge cases throughout the codebase. Great suggestions for improving the code quality! --- .../module_utils/common/controller_version.py | 79 ++++++++++++------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index 5899b4b3e..b625756b8 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -152,6 +152,27 @@ def refresh(self): def _get(self, item): return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) + def _validate_and_split_version(self): + """ + Validate version format and return split version parts. + + Expected formats: + w.x.y (3 parts) or w.x.y.z (4 parts) + + Returns: + list: Version parts split by '.' + + Raises: + ValueError: If version format is unexpected + """ + version_parts = self.version.split(".") + if len(version_parts) not in [3, 4]: + msg = f"{self.class_name}._validate_and_split_version: " + msg += f"Unexpected version format '{self.version}'. " + msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts" + raise ValueError(msg) + return version_parts + @property def dev(self): """ @@ -256,76 +277,74 @@ def uuid(self): def version(self): """ Return the controller version, if it exists. - Return None otherwise + Raise ValueError if version is not available. Possible values: version, e.g. "12.1.2e" - None """ - return self._get("version") + version = self._get("version") + if version is None: + msg = f"{self.class_name}.version: " + msg += "Version information not available in controller response" + raise ValueError(msg) + return version @property def version_major(self): """ - Return the controller major version as a string, if it exists. - Return None otherwise + Return the controller major version as a string. + Raise ValueError if version format is unexpected. We are assuming semantic versioning based on: https://semver.org + Expected formats: + w.x.y (3 parts) or w.x.y.z (4 parts) + Possible values: if version is 12.1.2e, return "12" if version is 12.4.1.245, return "12" - None """ - if self.version is None: - return None - version_parts = self.version.split(".") - if len(version_parts) >= 1: - return version_parts[0] - return None + version_parts = self._validate_and_split_version() + return version_parts[0] @property def version_minor(self): """ - Return the controller minor version as a string, if it exists. - Return None otherwise + Return the controller minor version as a string. + Raise ValueError if version format is unexpected. We are assuming semantic versioning based on: https://semver.org + Expected formats: + w.x.y (3 parts) or w.x.y.z (4 parts) + Possible values: if version is 12.1.2e, return "1" if version is 12.4.1.245, return "4" - None """ - if self.version is None: - return None - version_parts = self.version.split(".") - if len(version_parts) >= 2: - return version_parts[1] - return None + version_parts = self._validate_and_split_version() + return version_parts[1] @property def version_patch(self): """ - Return the controller patch version as a string, if it exists. - Return None otherwise + Return the controller patch version as a string. + Raise ValueError if version format is unexpected. We are assuming semantic versioning based on: https://semver.org + Expected formats: + w.x.y (3 parts) or w.x.y.z (4 parts) + Possible values: if version is 12.1.2e, return "2e" if version is 12.4.1.245, return "1" - None """ - if self.version is None: - return None - version_parts = self.version.split(".") - if len(version_parts) >= 3: - return version_parts[2] - return None + version_parts = self._validate_and_split_version() + return version_parts[2] @property def is_controller_version_4x(self) -> bool: From 89f27a54b4bad261e84c4ef9078c67db579c3b7a Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Mon, 14 Jul 2025 15:06:52 -1000 Subject: [PATCH 6/9] Update ControllerVersion unit tests 1. Update ControllerValue unit tests 1a. Expect ValueError if None is returned. 1b. Add tests for ND 4.1 version format e.g. 12.4.1.245 1c. black -l 160 reformatted import statements --- .../common/fixtures/responses_ep_version.json | 64 ++++++++++++ .../common/test_controller_version.py | 97 ++++++++++++------- 2 files changed, 125 insertions(+), 36 deletions(-) diff --git a/tests/unit/module_utils/common/fixtures/responses_ep_version.json b/tests/unit/module_utils/common/fixtures/responses_ep_version.json index 426030b47..a5c349e35 100644 --- a/tests/unit/module_utils/common/fixtures/responses_ep_version.json +++ b/tests/unit/module_utils/common/fixtures/responses_ep_version.json @@ -347,6 +347,22 @@ } }, "test_controller_version_00220b": { + "RETURN_CODE": 200, + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "MESSAGE": "OK", + "DATA": { + "version": "12.4.1.245", + "mode": "", + "isMediaController": "false", + "dev": "false", + "isHaEnabled": "false", + "install": "", + "uuid": "", + "is_upgrade_inprogress": "false" + } + }, + "test_controller_version_00220c": { "RETURN_CODE": 200, "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", @@ -377,6 +393,22 @@ } }, "test_controller_version_00230b": { + "RETURN_CODE": 200, + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "MESSAGE": "OK", + "DATA": { + "version": "12.4.1.245", + "mode": "", + "isMediaController": "false", + "dev": "false", + "isHaEnabled": "false", + "install": "", + "uuid": "", + "is_upgrade_inprogress": "false" + } + }, + "test_controller_version_00230c": { "RETURN_CODE": 200, "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", @@ -407,6 +439,22 @@ } }, "test_controller_version_00240b": { + "RETURN_CODE": 200, + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "MESSAGE": "OK", + "DATA": { + "version": "12.4.1.245", + "mode": "", + "isMediaController": "false", + "dev": "false", + "isHaEnabled": "false", + "install": "", + "uuid": "", + "is_upgrade_inprogress": "false" + } + }, + "test_controller_version_00240c": { "RETURN_CODE": 200, "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", @@ -437,6 +485,22 @@ } }, "test_controller_version_00250b": { + "RETURN_CODE": 200, + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "MESSAGE": "OK", + "DATA": { + "version": "12.4.1.245", + "mode": "", + "isMediaController": "false", + "dev": "false", + "isHaEnabled": "false", + "install": "", + "uuid": "", + "is_upgrade_inprogress": "false" + } + }, + "test_controller_version_00250c": { "RETURN_CODE": 200, "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", diff --git a/tests/unit/module_utils/common/test_controller_version.py b/tests/unit/module_utils/common/test_controller_version.py index c5b819ca3..efb436fef 100644 --- a/tests/unit/module_utils/common/test_controller_version.py +++ b/tests/unit/module_utils/common/test_controller_version.py @@ -29,17 +29,18 @@ import inspect import pytest -from ansible_collections.cisco.dcnm.plugins.module_utils.common.exceptions import \ - ControllerResponseError -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.sender_file import \ - Sender +from ansible_collections.cisco.dcnm.plugins.module_utils.common.exceptions import ControllerResponseError +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.sender_file import Sender from ansible_collections.cisco.dcnm.tests.unit.module_utils.common.common_utils import ( - MockAnsibleModule, ResponseGenerator, controller_version_fixture, - does_not_raise, params, responses_ep_version) + MockAnsibleModule, + ResponseGenerator, + controller_version_fixture, + does_not_raise, + params, + responses_ep_version, +) def test_controller_version_00000(controller_version) -> None: @@ -634,13 +635,14 @@ def responses(): @pytest.mark.parametrize( - "key, expected", + "key, expected, raises", [ - ("test_controller_version_00220a", "12.1.3b"), - ("test_controller_version_00220b", None), + ("test_controller_version_00220a", "12.1.3b", False), + ("test_controller_version_00220b", "12.4.1.245", False), + ("test_controller_version_00220c", None, True), ], ) -def test_controller_version_00220(controller_version, key, expected) -> None: +def test_controller_version_00220(controller_version, key, expected, raises) -> None: """ ### Classes and Methods @@ -656,12 +658,13 @@ def test_controller_version_00220(controller_version, key, expected) -> None: ``version`` returns: - Its value, if the "version" key is present in the controller response. - - None, if the "version" key is absent from the controller response. + - ValueError, if the "version" key is absent from the controller response. ### Expected result - 1. test_controller_version_00220a == "12.1.3b" - 2. test_controller_version_00220b is None + 1. test_controller_version_00220a == "12.1.3b" (ND 3.x format) + 2. test_controller_version_00220b == "12.4.1.245" (ND 4.1 format) + 2. test_controller_version_00220b == None (raises ValueError) """ def responses(): @@ -680,17 +683,22 @@ def responses(): instance = controller_version instance.rest_send = rest_send instance.refresh() - assert instance.version == expected + if not raises: + assert instance.version == expected + else: + match = "ControllerVersion.version: Version information not available in controller response" + pytest.raises(ValueError, match=match) @pytest.mark.parametrize( - "key, expected", + "key, expected, raises", [ - ("test_controller_version_00230a", "12"), - ("test_controller_version_00230b", None), + ("test_controller_version_00230a", "12", False), + ("test_controller_version_00230b", "12", False), + ("test_controller_version_00230c", None, True), ], ) -def test_controller_version_00230(controller_version, key, expected) -> None: +def test_controller_version_00230(controller_version, key, expected, raises) -> None: """ ### Classes and Methods @@ -709,7 +717,8 @@ def test_controller_version_00230(controller_version, key, expected) -> None: ### Expected result 1. test_controller_version_00230a == "12" - 2. test_controller_version_00230b is None + 2. test_controller_version_00230b == "12" + 2. test_controller_version_00230c == None (raises ValueError) """ def responses(): @@ -728,17 +737,22 @@ def responses(): instance = controller_version instance.rest_send = rest_send instance.refresh() - assert instance.version_major == expected + if not raises: + assert instance.version_major == expected + else: + match = "ControllerVersion.version: Version information not available in controller response" + pytest.raises(ValueError, match=match) @pytest.mark.parametrize( - "key, expected", + "key, expected, raises", [ - ("test_controller_version_00240a", "1"), - ("test_controller_version_00240b", None), + ("test_controller_version_00240a", "1", False), + ("test_controller_version_00240b", "4", False), + ("test_controller_version_00240c", None, True), ], ) -def test_controller_version_00240(controller_version, key, expected) -> None: +def test_controller_version_00240(controller_version, key, expected, raises) -> None: """ ### Classes and Methods @@ -758,7 +772,8 @@ def test_controller_version_00240(controller_version, key, expected) -> None: ### Expected result 1. test_controller_version_00240a == "1" - 2. test_controller_version_00240b is None + 1. test_controller_version_00240b == "4" + 2. test_controller_version_00240c == None (raises ValueError) """ def responses(): @@ -777,17 +792,22 @@ def responses(): instance = controller_version instance.rest_send = rest_send instance.refresh() - assert instance.version_minor == expected + if not raises: + assert instance.version_minor == expected + else: + match = "ControllerVersion.version: Version information not available in controller response" + pytest.raises(ValueError, match=match) @pytest.mark.parametrize( - "key, expected", + "key, expected, raises", [ - ("test_controller_version_00250a", "3b"), - ("test_controller_version_00250b", None), + ("test_controller_version_00250a", "3b", False), + ("test_controller_version_00250b", "1", False), + ("test_controller_version_00250c", None, True), ], ) -def test_controller_version_00250(controller_version, key, expected) -> None: +def test_controller_version_00250(controller_version, key, expected, raises) -> None: """ ### Classes and Methods @@ -807,7 +827,8 @@ def test_controller_version_00250(controller_version, key, expected) -> None: ### Expected result 1. test_controller_version_00250a == "3b" - 2. test_controller_version_00250b is None + 2. test_controller_version_00250b == "1" + 3. test_controller_version_00250c == None (raises ValueError) """ def responses(): @@ -826,7 +847,11 @@ def responses(): instance = controller_version instance.rest_send = rest_send instance.refresh() - assert instance.version_patch == expected + if not raises: + assert instance.version_patch == expected + else: + match = "ControllerVersion.version: Version information not available in controller response" + pytest.raises(ValueError, match=match) @pytest.mark.parametrize( From bee3f54527d162b46b372167c9e4c4f343a24800 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Tue, 15 Jul 2025 06:57:00 -1000 Subject: [PATCH 7/9] Remove needless handing of non-digit characters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit arobel > Couple more changes to make to the method is_controller_version_4x in file @plugins/module_utils/common/controller_version.py 1. On line 364, we should raise ValueError if either major or minor are None since we expect this never to happen. 2. There is no need for the code in the else block at line 367 since self.version_minor will never contain non-digit characters (only self.version_patch might contain non-digit characters). ⎿ Read plugins/module_utils/common/controller_version.py (397 lines) Claude > Excellent observations! The final changes make the is_controller_version_4x method much cleaner and more reliable: Key Improvements Made 1. Fail Early on None Values (lines 364-368): - Added explicit check to raise ValueError if major or minor are None - This should never happen due to our early validation, but if it does, we want to know immediately - Clear error message indicating what went wrong 2. Simplified Logic (lines 370-374): - Removed unnecessary string parsing logic for extracting numeric parts - Since version_minor is always the second part of a validated version string, it will always be numeric - Only version_patch might contain letters (like "2e"), but we don't use that in this method Benefits of These Changes - Cleaner Code: Removed ~15 lines of unnecessary string parsing logic - More Reliable: Direct integer conversion of version_minor since it's guaranteed to be numeric - Better Error Handling: Explicit failure if we encounter unexpected None values - Consistent with Design: Follows the "fail early" principle throughout the class Logic Summary The method now simply: 1. Gets major and minor version parts (guaranteed to be non-None strings) 2. Converts them directly to integers 3. Returns False if major=12 and minor<3, otherwise True This correctly identifies: - ND3 versions like "12.1.2e", "12.2.1e" → False (not 4.x) - ND4 versions like "12.4.1.245", "12.3.0" → True (4.x or higher) - Future versions like "13.0.0" → True (4.x or higher) The code is now much more robust, maintainable, and follows consistent error handling patterns throughout the class. Great suggestions for simplifying and improving the logic! --- .../module_utils/common/controller_version.py | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index b625756b8..8d090ae76 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -362,25 +362,16 @@ def is_controller_version_4x(self) -> bool: minor = self.version_minor if major is None or minor is None: - # If we can't determine version, assume it's a newer version - result = True + # This should never happen due to early validation, but if it does, raise an error + msg = f"{self.class_name}.{method_name}: " + msg += f"Unexpected None values: major={major}, minor={minor}" + raise ValueError(msg) + + # version_minor is always numeric, so we can convert directly to int + if int(major) == 12 and int(minor) < 3: + result = False else: - # Extract numeric part only from minor version in case of formats like "2e" - minor_numeric = "" - for char in minor: - if char.isdigit(): - minor_numeric += char - else: - break - - if not minor_numeric: - # If no numeric part found, assume it's a newer version - result = True - else: - if int(major) == 12 and int(minor_numeric) < 3: - result = False - else: - result = True + result = True except (ValueError, TypeError) as e: # If version parsing fails, assume it's a newer version msg = f"{self.class_name}.{method_name}: " From 4fcbe21603464208628162d587ce3657eb39ae1b Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Tue, 15 Jul 2025 07:03:33 -1000 Subject: [PATCH 8/9] Do not assume newer version if parsing fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit arobel > One final change to is_controller_version_4x method in @plugins/module_utils/common/controller_version.py If parsing fails (ValueError, TypeError) on line 375, we should re-raise this exception as a ValueError rather than return True. We do not want to assume the version if we cannot determine the version. ⎿ Read plugins/module_utils/common/controller_version.py (388 lines) Claude > Summary Perfect final change! This completes the robust error handling approach throughout the class: Key Change Made Re-raise Exceptions Instead of Assuming (lines 375-379): - Changed from logging warning and returning True to re-raising the exception as ValueError - Uses raise ValueError(msg) from e to preserve the original exception chain - Clear error message indicating what went wrong during version parsing Benefits of This Final Change 1. Consistent Error Handling: All methods now fail explicitly rather than making assumptions 2. No Silent Failures: If version parsing fails, the caller knows immediately 3. Proper Exception Chaining: The original exception is preserved using from e 4. Clear Error Messages: Descriptive error message indicating the specific failure Complete Error Handling Strategy The class now follows a comprehensive "fail early and explicitly" approach: 1. refresh(): Validates response data format immediately 2. version: Fails if version information is not available 3. _validate_and_split_version(): Validates version format (3 or 4 parts) 4. version_major/minor/patch: Use common validation, guaranteed to work 5. is_controller_version_4x: Re-raises any parsing errors instead of assuming Error Flow If any step fails, the error propagates up with clear messages: - Response format issues: "Expected response data to be a dictionary" - Missing version: "Version information not available" - Invalid format: "Unexpected version format '12.1'. Expected 3 or 4 parts" - Parsing errors: "Error parsing version 12.1.2e: invalid literal for int()" This approach ensures that: - Problems are detected at the earliest possible point - All failures provide clear, actionable error messages - No assumptions are made about version information - The calling code can handle errors appropriately The is_controller_version_4x method now properly fails if it cannot determine the version, which is much safer than silently assuming it's a newer version. This fix should resolve the original traceback issue while maintaining robust error handling throughout the class. --- plugins/module_utils/common/controller_version.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index 8d090ae76..e104c049a 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -373,11 +373,10 @@ def is_controller_version_4x(self) -> bool: else: result = True except (ValueError, TypeError) as e: - # If version parsing fails, assume it's a newer version + # If version parsing fails, re-raise as ValueError - do not assume version msg = f"{self.class_name}.{method_name}: " - msg += f"Error parsing version {self.version}: {e}. Assuming version 4.x" - self.log.warning(msg) - result = True + msg += f"Error parsing version {self.version}: {e}" + raise ValueError(msg) from e msg = f"{self.class_name}.{method_name}: " msg += f"self.version: {self.version}, " From c497c3ceed62373e09befee05640b860991ab368 Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Tue, 15 Jul 2025 08:50:02 -1000 Subject: [PATCH 9/9] UT: Fix unit test asserts for ControllerVersion 1. tests/unit/module_utils/common/test_controller_version.py 1a. We were not correctly checking for ValueError in the instance.version_* tests 1b. mode test. Modified to validate that ValueError is raised if mode is missing in the controller response 2. plugins/module_utils/common/controller_version.py 2a. Modify _get() to return None only if a parameter is missing in the controller response. 2b. @mode, modify to raise ValueError if the value is None (indicating key is missing in the controller response) 2c. Update docstrings and comments. 3. tests/unit/module_utils/common/fixtures/responses_ep_version.json 3a. Sorry, large diff. Sorted the JSON. 3b. Added TEST_NOTES to several of the mode fixtures (and added fixture for ND 4) - test_controller_version_00200a - test_controller_version_00200b - test_controller_version_00200c --- .../module_utils/common/controller_version.py | 44 +- .../common/fixtures/responses_ep_version.json | 709 +++++++++--------- .../common/test_controller_version.py | 41 +- 3 files changed, 431 insertions(+), 363 deletions(-) diff --git a/plugins/module_utils/common/controller_version.py b/plugins/module_utils/common/controller_version.py index e104c049a..a8d88fb78 100644 --- a/plugins/module_utils/common/controller_version.py +++ b/plugins/module_utils/common/controller_version.py @@ -150,7 +150,21 @@ def refresh(self): raise ValueError(msg) def _get(self, item): - return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item))) + """ + # Summary + + Return the parameter (item) from the response + + ## Notes + + - With ND 4, parameters like "mode", "uuid" are empty strings. + Return empty string in this case, rather than None. + - None indicates that the parameter is missing in the response (i.e. an error) + """ + value = self.response_data.get(item) + if value == "": + return "" + return self.conversion.make_none(self.conversion.make_boolean(value)) def _validate_and_split_version(self): """ @@ -252,14 +266,24 @@ def response_data(self): @property def mode(self): """ + # Summary + Return the controller mode, if it exists. Return None otherwise Possible values: LAN - None + "" + + ## Notes + + - mode will be "" for ND 4 """ - return self._get("mode") + value = self._get("mode") + if value is None: + msg = "Controller response is missing 'mode' parameter." + raise ValueError(msg) + return value @property def uuid(self): @@ -276,11 +300,13 @@ def uuid(self): @property def version(self): """ - Return the controller version, if it exists. - Raise ValueError if version is not available. + # Summary + + - Return the controller version, if it exists. + - Raise ValueError if version is not available. Possible values: - version, e.g. "12.1.2e" + version, e.g. "12.1.2e" or "12.4.1.245" """ version = self._get("version") if version is None: @@ -349,10 +375,14 @@ def version_patch(self): @property def is_controller_version_4x(self) -> bool: """ - ### Summary + # Summary - Return True if the controller version implies ND 4.0 or higher. - Return False otherwise. + + ## Raises + + - ValueError if unable to determine version """ method_name = inspect.stack()[0][3] diff --git a/tests/unit/module_utils/common/fixtures/responses_ep_version.json b/tests/unit/module_utils/common/fixtures/responses_ep_version.json index a5c349e35..f4718cdb2 100644 --- a/tests/unit/module_utils/common/fixtures/responses_ep_version.json +++ b/tests/unit/module_utils/common/fixtures/responses_ep_version.json @@ -1,555 +1,580 @@ { "test_controller_version_00100a": { - "RETURN_CODE": 200, - "METHOD": "GET", - "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00100b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00100b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "true", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00100c": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00100c": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00110a": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00110a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00110b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00110b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00120a": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00120a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "true", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "true", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00120b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00120b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00120c": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00120c": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00130a": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00130a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "true", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "true", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00130b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00130b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00130c": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00130c": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00140a": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00140a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "true", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "true" - } - }, - "test_controller_version_00140b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00140b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00140c": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00140c": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", - "uuid": "" - } - }, - "test_controller_version_00150a": { - "RETURN_CODE": 200, + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", + "uuid": "", + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00150a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } + "version": "12.1.3b" + }, + "MESSAGE": "OK", + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "RETURN_CODE": 200 }, "test_controller_version_00160a": { - "RETURN_CODE": 200, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK" + "RETURN_CODE": 200 }, "test_controller_version_00170a": { - "RETURN_CODE": 200, - "METHOD": "GET", - "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } + "version": "12.1.3b" + }, + "MESSAGE": "OK", + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "RETURN_CODE": 200 }, "test_controller_version_00180a": { - "RETURN_CODE": 404, + "DATA": {}, + "MESSAGE": "Not Found", "METHOD": "GET", "REQUEST_PATH": "https://foo/noop", - "MESSAGE": "Not Found", - "DATA": {} + "RETURN_CODE": 404 }, "test_controller_version_00190a": { - "RETURN_CODE": 500, + "DATA": {}, + "MESSAGE": "Internal Server Error", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "Internal Server Error", - "DATA": {} + "RETURN_CODE": 500 }, "test_controller_version_00200a": { - "RETURN_CODE": 200, - "METHOD": "GET", - "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "TEST_NOTES": [ + "mode key is populated. ND 3." + ], "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } + "version": "12.1.3b" + }, + "MESSAGE": "OK", + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "RETURN_CODE": 200 }, "test_controller_version_00200b": { - "RETURN_CODE": 200, + "TEST_NOTES": [ + "mode key is empty. ND 4." + ], + "DATA": { + "dev": false, + "install": "", + "is_upgrade_inprogress": false, + "isHaEnabled": false, + "isMediaController": false, + "mode": "", + "uuid": "", + "version": "12.4.1.245" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00200c": { + "TEST_NOTES": [ + "mode key is missing. Negative test." + ], "DATA": { - "version": "12.1.3b", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", + "isMediaController": "false", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00210a": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00210a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "foo-uuid", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00210b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00210b": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00220a": { - "RETURN_CODE": 200, + "isMediaController": "false", + "mode": "LAN", + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00220a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00220b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00220b": { "DATA": { - "version": "12.4.1.245", - "mode": "", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00220c": { - "RETURN_CODE": 200, + "version": "12.4.1.245" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00220c": { "DATA": { - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", - "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00230a": { - "RETURN_CODE": 200, + "isMediaController": "false", + "mode": "LAN", + "uuid": "" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00230a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00230b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00230b": { "DATA": { - "version": "12.4.1.245", - "mode": "", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00230c": { - "RETURN_CODE": 200, + "version": "12.4.1.245" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00230c": { "DATA": { - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", - "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00240a": { - "RETURN_CODE": 200, + "isMediaController": "false", + "mode": "LAN", + "uuid": "" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00240a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00240b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00240b": { "DATA": { - "version": "12.4.1.245", - "mode": "", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00240c": { - "RETURN_CODE": 200, + "version": "12.4.1.245" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00240c": { "DATA": { - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", - "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00250a": { - "RETURN_CODE": 200, + "isMediaController": "false", + "mode": "LAN", + "uuid": "" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00250a": { "DATA": { - "version": "12.1.3b", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00250b": { - "RETURN_CODE": 200, + "version": "12.1.3b" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00250b": { "DATA": { - "version": "12.4.1.245", - "mode": "", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00250c": { - "RETURN_CODE": 200, + "version": "12.4.1.245" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00250c": { "DATA": { - "mode": "LAN", - "isMediaController": "false", "dev": "false", + "is_upgrade_inprogress": "false", "isHaEnabled": "false", - "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00300a": { - "TEST_NOTES": [ - "12.2.2.238 implies ND 3.x" - ], - "RETURN_CODE": 200, + "isMediaController": "false", + "mode": "LAN", + "uuid": "" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200 + }, + "test_controller_version_00300a": { "DATA": { - "version": "12.2.2.238", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } - }, - "test_controller_version_00300b": { - "TEST_NOTES": [ - "12.3.1.248 implies ND 4.x" - ], - "RETURN_CODE": 200, + "version": "12.2.2.238" + }, + "MESSAGE": "OK", "METHOD": "GET", "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", - "MESSAGE": "OK", + "RETURN_CODE": 200, + "TEST_NOTES": [ + "12.2.2.238 implies ND 3.x" + ] + }, + "test_controller_version_00300b": { "DATA": { - "version": "12.3.1.248", - "mode": "LAN", - "isMediaController": "false", "dev": "false", - "isHaEnabled": "false", "install": "EASYFABRIC", + "is_upgrade_inprogress": "false", + "isHaEnabled": "false", + "isMediaController": "false", + "mode": "LAN", "uuid": "", - "is_upgrade_inprogress": "false" - } + "version": "12.3.1.248" + }, + "MESSAGE": "OK", + "METHOD": "GET", + "REQUEST_PATH": "https://172.22.150.244:443/appcenter/cisco/ndfc/api/v1/fm/about/version", + "RETURN_CODE": 200, + "TEST_NOTES": [ + "12.3.1.248 implies ND 4.x" + ] } } \ No newline at end of file diff --git a/tests/unit/module_utils/common/test_controller_version.py b/tests/unit/module_utils/common/test_controller_version.py index efb436fef..70e813bbf 100644 --- a/tests/unit/module_utils/common/test_controller_version.py +++ b/tests/unit/module_utils/common/test_controller_version.py @@ -534,34 +534,38 @@ def responses(): @pytest.mark.parametrize( - "key, expected", + "key, expected, raises", [ - ("test_controller_version_00200a", "LAN"), - ("test_controller_version_00200b", None), + ("test_controller_version_00200a", "LAN", False), + ("test_controller_version_00200b", "", False), + ("test_controller_version_00200c", None, True), ], ) -def test_controller_version_00200(controller_version, key, expected) -> None: +def test_controller_version_00200(controller_version, key, expected, raises) -> None: """ ### Classes and Methods - - ``ControllerVersion()`` - - ``refresh`` - - ``mode`` + - `ControllerVersion()` + - `refresh` + - `mode` ### Test - - ``mode`` returns expected values. + - `mode` returns expected values. + - ValueError is raised if `mode` is missing in the response ### Description ``mode`` returns: - Its value, if the "mode" key is present in the controller response. - - None, if the "mode" key is absent from the controller response. + - For ND 4, mode will be an empty string (""). + - ValueError if the "mode" key is absent from the controller response. ### Expected results 1. test_controller_version_00200a == "LAN" - 2. test_controller_version_00200b is None + 2. test_controller_version_00200b == "" + 3. test_controller_version_00200c == None (ValueError) """ def responses(): @@ -580,7 +584,13 @@ def responses(): instance = controller_version instance.rest_send = rest_send instance.refresh() - assert instance.mode == expected + print(f"raises: {raises}") + if not raises: + assert instance.mode == expected + else: + match = "Controller response is missing 'mode' parameter." + with pytest.raises(ValueError, match=match): + instance.mode # pylint: disable=pointless-statement @pytest.mark.parametrize( @@ -741,7 +751,8 @@ def responses(): assert instance.version_major == expected else: match = "ControllerVersion.version: Version information not available in controller response" - pytest.raises(ValueError, match=match) + with pytest.raises(ValueError, match=match): + instance.version_major # pylint: disable=pointless-statement @pytest.mark.parametrize( @@ -796,7 +807,8 @@ def responses(): assert instance.version_minor == expected else: match = "ControllerVersion.version: Version information not available in controller response" - pytest.raises(ValueError, match=match) + with pytest.raises(ValueError, match=match): + instance.version_minor # pylint: disable=pointless-statement @pytest.mark.parametrize( @@ -851,7 +863,8 @@ def responses(): assert instance.version_patch == expected else: match = "ControllerVersion.version: Version information not available in controller response" - pytest.raises(ValueError, match=match) + with pytest.raises(ValueError, match=match): + instance.version_patch # pylint: disable=pointless-statement @pytest.mark.parametrize(