Conversation
This commit adds the following: - Protocol definitions for Sender and ResponseHandler implementations - Concrete implementations of Sender (sender_nd.py) and ResponseHandler (response_handler_nd.py) - Implementation of Results (results.py) - Implementation of RestSend (rest_send.py) - New version of NDModule (nd_v2.py) - Enum definitions for HttpVerbEnum and OperationType - Added __init__.py to fix package import errors - A demo/POC basic module leveraging NDModule in nd_v2.py (modules/nd_rest_send_test.py)
1. Run linters on all files in this commit black -l 159 isort 2. plugins/module_utils/third_party/pydantic.py - Mock classes to satisfy Ansible sanity test environment requirements. 3. plugins/module_utils/pydantic_compat.py - Compatibility shim to reduce boilerplate in files that import Pydantic 4. nd_v2.py - Leverage pydantic compatibility shim - Remove commented imports
I wasn’t aware we still support Python 3.8 in this repository. Added required imports from typing and downgraded Python 3.9+ type-hint style to archaic style.
PEP 544 specifies an ellipsis constant as a placeholder in Protocol definitions. Pylint treats this as an error/warning. Added below directive to suppress this warning to all Protocol definitioin files: # pylint: disable=unnecessary-ellipsis
1. plugins/module_utils/log.py A modified version of log_v2.py from the DCNM Ansible Collection. 2. plugins/module_utils/logging_config.json Standard logging DictConfig with modifications for the ND Ansible Collection 3. plugins/modules/nd_rest_send_test.py Updated to import Log and initialize a logger
This is just for demo purposes to verify the RestSend architecture.
Several files copied from the DCNM Collection required only the copyright message modified to match the ND Collection (files that require additional changes are not included in this commit). We also removed a commented relative import from some of these files.
1. Align copyright with ND Collection 2. Fix type errors in original code imported from nd.py 3. Update quoted strings from original nd.py code to use f-string rather than format() 4. Update docstring Usage example to fix import
1. Previous modifications allow for the removal of mypy type suppression directives on import statements. Removed these where applicable. 2. After updating (local) copy of pyproject.toml to include pylint, black, line length constraint (159 characters), pylint disable directive is no longer needed. Will discuss with team whether we can add these to the repository’s pyproject.toml. 3. Update class docstring for NDModule.
This is mostly a copy from the DCNM Collection that includes more robust compatibility for pydantic objects when running ansible-test. There are several objects in the original that I’m not including since they are not used by existing code. We can add these later, if needed.
1. Added BooleanStringEnum from the SmartEndpoints demo since this PR will include the Smart Endpoints (see next commit…).
Initial Smart Endpoint files from the Smart Endpoints demo.
This commit focuses on refactoring the base path definitions into separate files for analyze, infra, manage, onemanage, etc, starting with infra. It also adds endpoints for infra/clusterhealth/[config | status] along with their supported query parameters. 1. Extract base path constants to base_path.py 2. Refactor infra into file base_paths_infra.py 3. Remove base_paths.py 4. Update endpoint_mixins.py and endpoint_query_params.py to include parameters used by clusterhealth endpoints. 5. Add clusterhealth endpoint definitions ep_api_v1_infra_clusterhealth.py 6. Update modules/nd_rest_send_test.py demo module to leverage clusterheath endpoint.
Limit to a single endpoint for better clarity.
Modify the demo module to leverage the Results class for standardized module output.
Forgot to run the demo module through back, isort, etc.
Pylint in VS code still complains about the following after narrow-typing response_data: unsupported-membership-test unsubscriptable-object Added a suppression directive since pylint (outside of VS code) is happy, so this seems to be a limitation with the VS Code pylint extension.
1. plugins/module_utils/response_handler_nd.py Addressed too-many-return-statements in code copied from original nd.py by populating an accumulator var ‘msg’ and returning it at the bottom of the method. 2. Add type hints to remaining untyped method signatures.
The initial fallback implementation used a combination of the following: module_utils/third_parth/pydantic.py and: module_utils/pydantic_compat.py This commit does the following: 1. Adds all the missing objects from plugins/module_utils/third_party/pydantic.py to plugins/module_utils/pydantic_compat.py. 2. Removes module_utils/third_party/pydantic.py The following objects were added: AfterValidator - Function that returns the validator function unchanged (fallback) BeforeValidator - Function that returns the validator function unchanged (fallback) PydanticExperimentalWarning - Aliased to the built-in Warning class StrictBool - Aliased to the built-in bool type ValidationError - Custom exception class with message support model_validator - Decorator that returns the function unchanged (fallback) validator - Decorator that returns the function unchanged (fallback) All objects now have: Real Pydantic imports when available (in the try block and TYPE_CHECKING block) Fallback implementations when Pydantic is not available (in the except ImportError block) Exports in the __all__ list The pydantic_compat.py module now provides a complete compatibility layer matching all the objects from the third_party pydantic module.
Renamed NDErrorData properties data and payload to request_payload and response_payload, respectively. Summary of changes: 1. plugins/module_utils/nd_v2.py - Renamed data → request_payload - Renamed payload → response_payload - Updated docstring attributes 2. plugins/modules/nd_rest_send_test.py - Updated error.payload → error.response_payload Benefits request_payload: Clearly indicates the data that was sent TO the controller response_payload: Clearly indicates the data received FROM the controller
1. Add unit test infrastructure. - tests/unit/module_utils/response_generator.py Response generator for unit tests - tests/unit/module_utils/fixture.py Function to load test inputs from JSON files - tests/unit/module_utils/common_utils.py Common utilities used by unit tests - __init__.py where required
Since we already verify that mandatory instance variables are set in their getter properties, the checks in the _commit_* methods are redundant and can be removed.
ResponseHandler has already formatted response and result structures. Leverage these in the demo module.
Previously, rest_send was private, which is not what the contract for NDModule should be. Added a public getter property to expose it.
I’d forgotten to copy the query_params.py file over from the DCNM collection, so Claude was not leveraging the composition pattern defined in this file when building the endpoints. 1. Added this file 2. Asked Claude to refactor existing infra and manage endpoints to leverage it to compose query and lucene params into the query string.
Forgot to update the boilerplate at the top of the file.
Since RestSend is v1 in the ND collection, I’d updated its implements property to “rest_send_v1” but forgot to update the unit test that verifies this propery.
Address review comments: #180 (comment)
| from ansible_collections.cisco.nd.plugins.module_utils.ep.base_path import ND_INFRA_API | ||
|
|
||
|
|
||
| class BasePath: |
There was a problem hiding this comment.
why do we define the same BasePath class in multiple files? this seems like something you would like to avoid to me since it can lead to issues with importing, but can also lead to confusion in my opinion
There was a problem hiding this comment.
This is common practice in at least a few popular libraries e.g. Django alone has Model defined conceptually across dozens of app modules. What would be the confusion given that the file name becomes part of the import statement (and, hence, clarifies the BasePath being used)? Also, each BasePath class has a full docstring visible when hovering over it in an IDE (including a Usage section) so wouldn't that mitigate any confusion?
There was a problem hiding this comment.
Ok, i would argue that you would never use same classNames since a class is a definition of something. Why would you define it twice with the same name if it is not the same thing? Also if you would import the two of these classNames in order you require alias importing. But if you find that this is common practice then it is fine by me.
For this class, parameters were not being validated due to missing model_config.
Adding the following AI-generated code analyses for context during code review discussions. - Overall analysis for nd.py vs nd_v2.py (SOLID best practices) - ANALYSIS_SOLID_NDv2.md - ANALYSIS_SOLID_ND.md - Endpoint implementations analysis (design, CC and cognitive load) - plugins/module_utils/ep/ANALYSIS_COMPLEXITY.md - plugins/module_utils/ep/ANALYSIS_ENDPOINT_DESIGN.md - plugins/module_utils/ep/ANALYSIS_TYPE_SAFETY.md
# Update ResponseHandler to handle additional HTTP status codes ## Summary Extended ResponseHandler to explicitly handle HTTP status codes that appear in the manage.json API schema but were previously treated as generic errors. Includes comprehensive unit tests for all new status code handling. ## Changes - **Added 207 (Multi-Status)** to `RETURN_CODES_SUCCESS` set - GET requests: treated as success with `found=True` - POST/PUT/DELETE requests: treated as success with `changed=True` - **Added explicit error handling** for new `RETURN_CODES_ERROR` set containing: - **405 (Method Not Allowed)**: configuration/logic error - **409 (Conflict)**: resource conflict condition - **Updated docstrings** in `_handle_get_response()` and `_handle_post_put_delete_response()` to document the new status codes ## Unit Tests Added 7 new tests to comprehensively cover the new status codes: - **1 Updated test**: test_response_handler_nd_00010 - initialization now verifies new status code sets - **3 New GET tests**: test_response_handler_nd_00535 (207), 00575 (405), 00580 (409) - **3 New POST/PUT/DELETE tests**: test_response_handler_nd_00655 (207), 00695 (405), 00705 (409) **Test Results**: All 55 tests passing (was 49 tests) ## Code Quality - Code passes all linters: black -l 159 ✓, isort ✓, pylint ready - All tests passing with 100% success rate - Test execution time: 0.05s ## Files Modified - `plugins/module_utils/response_handler_nd.py` - `tests/unit/module_utils/test_response_handler_nd.py`
Strawman dependency lock for uv.
| self.log.debug(msg) | ||
|
|
||
| # Early exit for read-only operations | ||
| if self._current.check_mode or self._current.operation_type.is_read_only() or self._current.state == "query": |
There was a problem hiding this comment.
why would both self._current.operation_type.is_read_only() or self._current.state == "query" be checked? is the smart enum pattern with the is_read_only() not suffficient?
another thing which I am wondering is why check_mode here and not already before the function call, since checked_mode or arguably query would never result in a change. so following your focus on single responsibility did you consider splitting the functionality into ("should changes be possible?") and a detector ("did changes happen?")?
There was a problem hiding this comment.
Yes, this is one of the things that I wanted to change but didn't have time to adequately test.
And, yes, agree that we can optimize this logic. As some history, the enum was added fairly recently in the DCNM collection and was intended to replace any other inputs to conditionals (like _current_state) so that (ideally) we have a single thing to check.
There was a problem hiding this comment.
Do we track this somewhere? Or do we look at doing this in the restsend PR?
Summary
1. Addressed Akini’s review comment by fixing the timeout decrement issue in RestSend._commit_normal_mode
2-3. Updated unit tests to reflect the above modification.
Detail
1. Fixed the timeout logic in rest_send.py
Changed the timeout decrement to only happen when a request fails (when success = False), and separated the sleep from the timeout decrement:
Before:
Timeout was decremented at the start of every loop iteration, wasting the retry budget on the first request
After:
- Timeout only decrements when success = False (request failed)
- Sleep only happens when unit_test = False (normal mode)
- Timeout always decrements on failure, regardless of unit_test mode
success = self.result_current["success"]
if success is False:
if self.unit_test is False:
sleep(self.send_interval)
timeout -= self.send_interval
# ... debug logging ...
2. Updated test_rest_send_00510
- Increased timeout from 1 to 10 seconds to allow retries
- Explicitly set send_interval = 5 for clarity
- Updated generator to provide 60 mock responses to handle potential retries (10 / 5 = 2 retries)
- Updated docstring to reflect retry behavior
3. Updated test_rest_send_00520
- Same changes as test_rest_send_00510
0 Increased timeout from 1 to 10 seconds
- Explicitly set send_interval = 5
- Updated generator to provide 60 mock responses
- Updated docstring to reflect retry behavior
Test Results: All 31 rest_send tests pass.
Adding analysis file. We can move this to an appropriate place under .claude once we come to a team consensus.
Rename endpoint classes to remove "V1" from class names since version is now encoded in the directory structure (ep/v1/). This simplifies imports and aligns with the directory-level versioning pattern established in the previous commit. ## Class Name Changes Endpoint classes renamed to remove version prefix: - EpApiV1InfraAaaLocalUsersGet → EpInfraAaaLocalUsersGet - EpApiV1InfraAaaLocalUsersPost → EpInfraAaaLocalUsersPost - EpApiV1InfraAaaLocalUsersPut → EpInfraAaaLocalUsersPut - EpApiV1InfraAaaLocalUsersDelete → EpInfraAaaLocalUsersDelete - EpApiV1InfraClusterhealthConfigGet → EpInfraClusterhealthConfigGet - EpApiV1InfraClusterhealthStatusGet → EpInfraClusterhealthStatusGet - EpApiV1ManageSwitchesGet → EpManageSwitchesGet Base classes renamed similarly (removed _ApiV1_ from names). ## Import Path Examples OLD: from ansible_collections.cisco.nd.plugins.module_utils.ep.v1.ep_infra_aaa import EpApiV1InfraAaaLocalUsersGet NEW: from ansible_collections.cisco.nd.plugins.module_utils.ep.v1.ep_infra_aaa import EpInfraAaaLocalUsersGet Or via re-export: from ansible_collections.cisco.nd.plugins.module_utils.ep import EpInfraAaaLocalUsersGet ## Files Modified - plugins/module_utils/ep/v1/ep_infra_aaa.py (class names, Literal type hints) - plugins/module_utils/ep/v1/ep_infra_clusterhealth.py (class names) - plugins/module_utils/ep/v1/ep_manage_switches.py (class names, added Literal import) - plugins/module_utils/ep/v1/__init__.py (updated imports and __all__) - plugins/module_utils/ep/__init__.py (updated re-exports) - plugins/modules/000_TEST_MODULE.py (updated imports) - plugins/modules/nd_rest_send_test.py (updated imports) - tests/unit/module_utils/ep/test_*.py (updated class references) ## Quality Assurance ✅ All 161 unit tests pass ✅ Black formatting applied (159 char line length) ✅ isort import sorting verified ✅ pylint passes with 10.00/10 rating ✅ Three import patterns verified (explicit v1, re-export, direct) ✅ Version metadata (api_version, min_controller_version) accessible ✅ Backward compatibility maintained via ep/__init__.py re-exports ## Benefits 1. Cleaner class names (version not redundantly repeated) 2. Version information now in directory/import path 3. Ready for future v2/v3 support with minimal changes 4. Maintains full backward compatibility Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Replace all inspect.stack()[0][3] calls with static method name strings to eliminate runtime stack inspection overhead in production code: - plugins/module_utils/results.py: 11 method names (methods and property setters) - plugins/module_utils/sender_nd.py: 4 method names (property setters) + added _get_caller_name() helper for logging caller info in commit() - plugins/module_utils/log.py: 1 method name (property setter) The only remaining inspect.stack() call in production code is in the _get_caller_name() helper which is necessary for logging purposes. Unit test files left unchanged as performance is not a concern. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This document codifies the long-term strategy for supporting multiple ND REST API versions (v1, v2, v3, etc.) in the Cisco ND Ansible collection. It explains the current architecture, the rationale behind design decisions, and the planned next steps.
Replace simple string constants in base_path.py with an ApiPath string-based Enum for type safety, IDE autocomplete, and prevention of invalid path usage. ## Changes 1. Created ApiPath(str, Enum) with all 7 base API paths: - ApiPath.ANALYZE = "/api/v1/analyze" - ApiPath.INFRA = "/api/v1/infra" - ApiPath.MANAGE = "/api/v1/manage" - ApiPath.ONEMANAGE = "/api/v1/onemanage" - ApiPath.MSO = "/mso" - ApiPath.NDFC = "/appcenter/cisco/ndfc/api" - ApiPath.LOGIN = "/login" 2. Updated BasePath classes to use ApiPath.*.value: - plugins/module_utils/ep/v1/base_paths_infra.py - plugins/module_utils/ep/v1/base_paths_manage.py 3. Maintained backward compatibility: - Old constant names (ND_INFRA_API, etc.) still work - Created aliases: ND_INFRA_API = ApiPath.INFRA.value - All existing code continues to function 4. Updated tests to verify enum integration: - test_base_path.py: Added 3 new tests for enum functionality - test_base_paths_infra.py: Verify enum + backward compat - test_base_paths_manage.py: Verify enum + backward compat ## Benefits 1. **Type Safety** - Function parameters can require ApiPath type 2. **IDE Support** - Autocomplete shows all valid API paths 3. **Discoverability** - ApiPath. shows all available paths 4. **Future-Proof** - Easy to add v2 paths when needed 5. **Backward Compatible** - All existing code continues to work ## Quality Assurance ✅ All 164 endpoint tests pass (3 new tests added) ✅ Black formatting: OK ✅ pylint rating: 10.00/10 ✅ Backward compatibility verified ✅ Enum can be used directly in strings and f-strings ✅ BasePath classes properly integrated Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Relocates ResponseValidationStrategy Protocol and NdV1Strategy to protocol_response_validation.py and response_validation_nd_v1.py, consistent with the existing protocol_sender.py / protocol_response_handler.py naming convention. Removes the now-unnecessary response_strategies/ subpackage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Closing as superseeded by #185 |
PR: RestSend framework with Smart Endpoints and unit tests
NOTES
Ansible sanity tests pass with the following:
We could add tests/sanity/ignore-*.txt files to overcome this for stable-2.16 and stable-2.17, but (IMHO) the better solution would be to drop support for Ansible versions that include Python 2.7 and 3.7, 3.8 in their test matrix since nobody should be using these EOL Python versions anyway.
Current Ansible Support Levels
AIs
Summary
ansible-test, as shown in the example below:Changes
Core REST framework
RestSendclass for sending REST requests with retry logic, check mode support, and configurable timeout/send_intervalSenderclass implementingSenderProtocolfor sending requests via the Ansible HttpApi plugin, with response normalization for non-JSON responsesResponseHandlerclass implementingResponseHandlerProtocolfor parsing ND controller responses, including GET (found/success) vs POST/PUT/DELETE (changed/success) handling and extraction of 7 ND error message formatsResultsclass for aggregating task results across multiple REST operationsSmart Endpoints (plugins/module_utils/ep/v1)
/api/v1/infraand/api/v1/manageschema divisionsSupporting infrastructure
HttpVerbEnum,BooleanStringEnum,OperationTypeenumsUnit tests (Total 285 tests)
ResponseHandler:_handle_get_response()across all status codes (200, 201, 202, 204, 404, 400, 401, 500)_handle_post_put_delete_response()with ERROR key, DATA.error, success codes, error codeserror_messagefor all 7 ND error formats (raw_response, code/message, messages array, errors array, no DATA, non-dict DATA, unknown fallback)Sender:_normalize_response()for normal JSON, non-JSON raw, missing MESSAGE, missing rawcommit()with mocked Connection for GET/POST/PUT/DELETE, connection reuse, error wrappingRestSendwith check mode, retries, sequential commits, error conditionsTest plan
PYTHONPATH=<collections_root> python -m pytest tests/unit/black -l159 --check,isort --check-only,pylintUnit test results 2026-02-17 @14:39