WIP: dcnm_vrf - Pydantic integration #400
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
allenrobel
changed the title
Merge the following methods: - update_vrf_template_config_from_vrf_template_model - update_vrf_template_config_from_model Into the following method: - update_vrf_template_config_from_vrf_model Call this merged method from: - get_have - push_diff_create
- update_vrf_template_config_from_vrf_model() Originally, this method accepted a VrfObjectV12 model and returned a dict containing the modified vrfTemplateConfig field. Our overall goal is to convert inputs to models as early as possible, work on those models, and then output the models as JSON as late as possible. In this spirit, we’ve modified this method to accept a model and return a model. No changes were required to the methods that call this method. We’ve commented out some code which we’ll remove in the next commit if integration tests pass.
No functional changes in this commit. - After integration tests passed, remove commented code. - push_diff_create_update() - Update debug log message to include self.diff_create_update contents
1. plugins/module_utils/vrf/dcnm_vrf_v12.py - push_diff_create_update - return early if self.diff_create_update is empty
Previously, get_items_to_detach() was a private method within get_diff_delete(). However, get_items_to_detach() can also be leveraged by get_diff_override(). Hence, moving it to class scope.
No functional changes in this commit. Rename the following methods: - get_next_vlan_id_for_fabric - get_next_vrf_id To (respectively): - get_next_fabric_vlan_id - get_next_fabric_vrf_id Move these closer to the top of the file where the other utility methods live and update their docstrings.
1. plugins/module_utils/vrf/dcnm_vrf_v12.py - diff_for_attach_deploy want[isAttached] is being deleted within a conditional after it’s already been deleted outside the conditional. Removing the deletion within the conditional as it’s redundant.
1. plugins/module_utils/vrf/dcnm_vrf_v12.py - diff_for_attach_deploy Replace all instances of ast.literal_eval with json.loads
There was a problem hiding this comment.
Pull Request Overview
This PR integrates Pydantic-based validation into the DCNM VRF modules, reorganizes controller versions, adds enums for common parameters, and updates CI to install Pydantic.
- Replace in-code IP validation with Pydantic models and dedicated validator functions.
- Introduce separate DCNM v11 and NDFC v12 classes and tests.
- Add enums for Ansible states, HTTP requests, and BGP password encryption; update API client classes.
- Modify CI workflows to install the Pydantic dependency.
Reviewed Changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/module_utils/common/validators/ipv6_cidr_host.py | Add IPv6 CIDR-format host validator |
| plugins/module_utils/common/validators/ipv4_host.py | Add IPv4 host validator |
| plugins/module_utils/common/validators/ipv4_cidr_host.py | Add IPv4 CIDR-format host validator |
| plugins/module_utils/common/models/ipv6_host.py | Add Pydantic model for IPv6 host |
| plugins/module_utils/common/models/ipv6_cidr_host.py | Add Pydantic model for IPv6 CIDR host |
| plugins/module_utils/common/models/ipv4_host.py | Add Pydantic model for IPv4 host |
| plugins/module_utils/common/models/ipv4_cidr_host.py | Add Pydantic model for IPv4 CIDR host |
| plugins/module_utils/common/enums/http_requests.py | Add RequestVerb enum for HTTP verbs |
| plugins/module_utils/common/enums/bgp.py | Add BgpPasswordEncrypt enum |
| plugins/module_utils/common/enums/ansible.py | Add AnsibleStates enum |
| plugins/module_utils/common/api/v1/lan_fabric/rest/top_down/top_down.py | Add TopDown base API client class |
| plugins/module_utils/common/api/v1/lan_fabric/rest/top_down/fabrics/fabrics.py | Add Fabrics API client subclass |
| plugins/module_utils/common/api/v1/lan_fabric/rest/top_down/fabrics/vrfs/vrfs.py | Add Vrfs API client and endpoint classes |
| .github/workflows/main.yml | Update CI jobs to install Pydantic |
Comments suppressed due to low confidence (2)
plugins/module_utils/common/api/v1/lan_fabric/rest/top_down/fabrics/vrfs/vrfs.py:24
- The relative import uses nine dots (
.........common), which is likely incorrect. Verify the correct number of parent levels and adjust the import path.
from .........common.enums.http_requests import RequestVerb
.github/workflows/main.yml:36
- There's a stray dash (
-) before the sanity job definition, which may break YAML parsing. Remove the extra entry.
-
Similar to last commit, but for the remaining instances of ast.literal_eval. Also, remove ast import since it’s no longer needed.
This typos caught by Copilot PR review
Fix typo caught by Copilot PR review
Logic simplification suggested by Copilot PR review
Experimental commit. Refactor get_have() to extract have_create into new method populate_have_create() We’ve commented original code in get_have() so that we can revert if integration tests fail. Will clean up in next commit.
Fixed below issue. ERROR: plugins/module_utils/vrf/dcnm_vrf_v12.py:1178:1: W293: blank line contains whitespace
No functional changes in this commit. 1. get_have - Remove commented code 2. populate_have_create - Update docstring with more detail - Remove debug log messages 3. get_have - Update docstring to reference populate_have_create 4. get_diff_merge - Move comment regarding “Special cases” into diff_merge_create - Add a TODO to review with Mike why this cannot be moved to a method called by push_to_remote()
1. get_have - Rename curr_vrfs to current_vrfs_set - Simplify current_vrfs_set generation with set comprehension
1. get_have Extract the code responsible for populating self.have_deploy out of get_have and move it into populate_have_deploy. 2. populate_have_deploy - new method, called from get_have We’ve commented out this code in get_have and will remove it in the next commit if integration tests pass.
No functional changes in this commit. 1. get_have - Remove commented code after verifying successful integration tests
1. get_have Extract the code responsible for populating self.have_attach out of get_have and move it into populate_have_attach. 2. populate_have_attach - new method, called from get_have We’ve commented out the code in get_have that is refactored and will remove it in the next commit if integration tests pass.
No functional changes in this commit. Fix the following pylint complaint. ERROR: plugins/module_utils/vrf/dcnm_vrf_v12.py:1289:20: logging-fstring-interpolation: Use lazy % formatting in logging functions
1. Add method_name to fail_json message 2. Remove e_values variable by updating attach[extensionValues] directly
No functional changes in this commit. 1. get_have - Remove commented code on verifying successful integration tests after refactor
1. Refactor get_want into the following three methods - get_want_attach - get_want_create - get_want_deploy
No functional changes in this commit. 1. get_want - Remove commented code on verifying successful integration tests after refactor
1. get_diff_query The main if-else clause of the original method handles two cases for VRFs in self.fabric 1. if - Create a query-state diff for VRFs present in want 2. else - Create a query-state diff for all VRFs Refactor this into two methods, called from get_diff_query, that perform the actions described above. We are retaining the original get_diff_query code (commented-out) so that we can easily revert if integration tests fail. We will clean this up in the next commit.
1. tests/unit/module_utils/vrf/fixtures/model_payload_vrfs_attachments.json With the changes in the last commit, instacneValues in the fixture needs to be a dictionary. 2. plugins/module_utils/vrf/transmute_diff_attach_to_payload.py - Run through black -l 160 3. plugins/module_utils/vrf/model_payload_vrfs_attachments.py - field_validator needs to be a @classmethod - Run through black to appease pep8 and pylint
1. plugins/module_utils/vrf/transmute_diff_attach_to_payload.py - Simplify input attribute verification - Simplify list comprehension
1. tests/unit/modules/dcnm/fixtures/dcnm_vrf_12.json - Original name was dcnm_vrf.json - Renamed to dcnm_vrf_12.json for consistency with dcnm_vrf_11.json 2. tests/unit/modules/dcnm/test_dcnm_vrf_12.py - Update loadPlaybookData to load the new filename from 1 above.
1. plugins/module_utils/vrf/model_payload_vrfs_attachments.py Previously, this model accepted any string for extensionValues. This update provides validation and serialization for most fields within extensionValues. Some tweaking is still pending (e.g. validating DOT1Q_ID for which any string is currently accepted). Also, add copyright and module docstring 2. plugins/module_utils/vrf/transmute_diff_attach_to_payload.py - Leverage the change in 1 above. 3. tests/unit/modules/dcnm/fixtures/dcnm_vrf_12.json Update fixture to align with changes in 1 above (extensionValues should now be a dict).
The field serializer for extension_values in PayloadVrfsAttachmentsLanAttachListItem needs to set the value to “” if MULTISOTE_CONN and VRF_LITE_CONN both have zero length, else the controller chokes on the payload. This commit tests one way to accomplish this. If this works against the contoller, I’ll clean things up in the next commit.
1. plugins/module_utils/vrf/dcnm_vrf_v12.py - push_diff_attach_model Getting a different 500 error with latest changes. Let’s try running the model-generated payload through json.dumps(). The payload looks fine to me, but includes only single backslash escape characters rather than triple-backslash. Maybe json.dumps() will fix this. If so, will look into the model’s field_serializer for a proper solution.
Experimental commit. This seems to align the model-emitted payload with what a successful payload looks like. Fingers crossed! 1. plugins/module_utils/vrf/model_payload_vrfs_attachments.py - PayloadVrfsAttachments - Serialize AUTO_VRF_LITE_FLAG to a lower-case string - Run through black/pylint - Fix serialization of MULTISITE_CONN - Fix serialization of VRF_LITE_CONN - Fix extension_values field definition - Add @field_validator for extension_values - Fix serialization of extension_values - Fix serialization of instance_values 2. plugins/module_utils/vrf/transmute_diff_attach_to_payload.py - DiffAttachToControllerPayload - Use model_dump_json() when dumping the model. - May need to revert this based on integration test results…
1, plugins/module_utils/vrf/transmute_diff_attach_to_payload.py - DiffAttachToControllerPayload.commit() - Update assignment of self._payload - Update debug messages - Remove unused/commented code
Experimental commit. Pydantic is complaining about missing fields in controller responses. Given that the controller is always right, we’re modifying the model to make these fields optional.
1. plugins/module_utils/vrf/model_controller_response_vrfs_switches_v12.py
- ControllerResponseVrfsSwitchesSwitchDetails
- extension_values: make optional
- ControllerResponseVrfsSwitchesExtensionValuesOuter
- vrf_lite_conn: make optional
- multisite_conn: make optional
Error from Pydantic is:
2 validation errors for ControllerResponseVrfsSwitchesV12
DATA.0.switchDetailsList.0.extensionValues.VRF_LITE_CONN
Field required [type=missing, input_value={}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.9/missing
DATA.0.switchDetailsList.0.extensionValues.MULTISITE_CONN
Field required [type=missing, input_value={}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.9/missing
1. plugins/module_utils/vrf/transmute_diff_attach_to_payload.py 1a. self._payload is a list, but was type-hinted as a str. 1b commit() - When populating PayloadVrfsAttachments, set vrfName to “” if it’s None. 1c. Improve logging messages. 1d. Move assignment of vrf_lite_conn_list closer to where it is used. 2. tests/unit/modules/dcnm/test_dcnm_vrf_12.py 2a. test_dcnm_vrf_12_query_vrf_lite - Update to assert on new value for extensionValues 2b. test_dcnm_vrf_12_query_lite_without_config - Update to assert on new value for extensionValues 3. plugins/module_utils/vrf/model_controller_response_vrfs_switches_v12.py 3a. Make most fields optional with a default so that model_construct() can build a default representation of the model. 3b. VrfLiteConnOuterItem Sort of a hack, but now that we are returning a fully-formed model with default values for things not present in the switch response, we need a way to determine if interface(s) on the switch are VRF_LITE capable. Previously, we did this by comparing if extensionValues was None or “”. Now, we’ve set AUTO_VRF_LITE_FLAG to “NA” by default. So, if an interface is not VRF_LITE capable, this will now be “NA”. 4. plugins/module_utils/vrf/dcnm_vrf_v12.py 4a. _update_vrf_lite_extension_model Per 3b above, modify the conditional to inspect AUTO_VRF_LITE_FLAG for “NA” and skip extensionValues processing if this is the case. 5. plugins/module_utils/vrf/model_controller_response_vrfs_deployments_v12.py 5a. Make REQUEST_PATH optional with a default of “”
1. Log test titles into dcnm.log to correlate tests with debug output. - tests/integration/targets/dcnm_vrf/tests/dcnm/query.yaml - tests/integration/targets/dcnm_vrf/tests/dcnm/replaced.yaml
1. plugins/module_utils/vrf/serial_number_to_vrf_lite.py The name of the associated class changed from VrfLiteModel to PlaybookVrfLiteModel. Updating the class and commit method docstrings to match.
1. tests/unit/module_utils/vrf/fixtures/model_controller_response_fabrics_easy_fabric_get.json Fixture data 2. tests/unit/module_utils/vrf/fixtures/load_fixture.py 2a. Add method to load the fixture data in 1. 3. tests/unit/module_utils/vrf/test_controller_response_fabrics_easy_fabric_get.py Unit tests for the model 4. plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py 4a. ControllerResponseFabricsEasyFabricGet Initial base model 4b. ControllerResponseFabricsEasyFabricGetNvPairs nvPairs model (no unit tests yet, will add in the next commit)
ERROR: Found 3 pep8 issue(s) which need to be resolved: ERROR: plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py:4:1: E302: expected 2 blank lines, found 1 ERROR: plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py:338:1: E302: expected 2 blank lines, found 1 ERROR: plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py:341:1: W293: blank line contains whitespace
Update import ignores for plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py
1. tests/unit/module_utils/vrf/test_controller_response_fabrics_easy_fabric_get_nv_pairs.py 1a. ControllerResponseFabricsEasyFabricGetNvPairs Initial set of Windsurf-generated test cases. Initial scan looks adequate, but improvements are possible, especially in further constraining allowable values. This is a decent first cut though.
1. tests/unit/modules/dcnm/test_dcnm_vrf_12.py A couple fixtures were getting loaded that were not used. Remove these lines. 2. tests/unit/modules/dcnm/fixtures/dcnm_vrf_12.json 2a. Remove the fixtures referenced in 1 from the fixture file - fabric_details_mfd - fabric_details_vxlan
1. plugins/module_utils/common/enums/ansible.py Update docstring with detailed descriptions of the Ansible states used in the ansible-dcnm project.
1. plugins/module_utils/vrf/model_controller_response_fabrics_easy_fabric_get.py 1a. Add license boilerplate and module docstring
Based on conversations with the team, this commit: 1. Restore the original dcnm_vrf module, and its unit tests. 1a. Rename the original unit test functions test_dcnm_vrf_* -> test_dcnm_vrf_v1_* 2. Rename the new vrf module to dcnm_vrf_v2 2a. Rename unit test files for the new vrf module and DISABLE them for now These tests ALL broke for some reason. Disabling until I can figure it out. DISABLE_test_dcnm_vrf_v2_11.py DISABLE_test_dcnm_vrf_v2_12.py 2b Rename unit test functions for the new vrf module: test_dcnm_vrf_11_* -> test_dcnm_vrf_v2_11_* test_dcnm_vrf_12_* -> test_dcnm_vrf_v2_12_*
1. tests/sanity/ignore-*.txt 1a. Add dcnm_vrf_v2.py 2. Appease black linter 2a. tests/unit/modules/test_dcnm_vrf.py Add blank line at end of file. 2b. plugins/modules/dcnm_vrf.py Add blank line at end of file. 2c. plugins/modules/dcnm_vrf_v2.py Add blank lines after import statements.
1. plugins/module_utils/common/enums/ansible.py 1a. Fix pep8 error in ansible.py ERROR: Found 1 pep8 issue(s) which need to be resolved: ERROR: plugins/module_utils/common/enums/ansible.py:45:1: W293: blank line contains whitespace 1b. Use UPPERCASE keys in AnsibleStates enum to appease pylint
1. Fix and re-enable the unit tests for dcnm_vrf_v2 These tests were failing because the patch for dcnm_version_supported was still referencing dcnm_vrf, when it needed to reference dcnm_vrf_v2.
1. Multiple files. FIx typo in class name. PayloadfVrfsDeployments -> PayloadVrfsDeployments 2. tests/unit/module_utils/vrf/test_model_payload_vrfs_deployments.py Unit tests for PayloadVrfsDeployments were failing because we were passing value into the class via vrf_names rather than vrfNames. 3. tests/unit/module_utils/vrf/test_model_payload_vrfs_attachments.py Fix docstring typo.
allenrobel
added
duplicate
Collaborator
Author
|
Replacing this PR with #550 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR, when complete:
dcnm_vrfintegration tests for readabilityCaveats
Details
Will update as work progresses.
Added files
plugins/module_utils/common/enums/ansible.pyEnumerations related to Ansible.
plugins/module_utils/common/enums/bgp.pyplugins/module_utils/common/enums/request.pyEnumerations related to HTTP requests
plugins/module_utils/vrf/vrf_controller_to_playbook_v12.pyVrfControllerToPlaybookV12ModelSerialize NDFC version 12 controller payload fields to fields used in a dcnm_vrf playbook.
plugins/module_utils/vrf/vrf_controller_to_playbook.pyVrfControllerToPlaybookModelSerialize payload fields (common to NDFC versions 11 and 12) to fields
used in a dcnm_vrf playbook.
plugins/module_utils/vrf/vrf_playbook_model.pyValidation model for dcnm_vrf playbooks
plugins/module_utils/common/models/ipv4_cidr_host.pyModel to validate a CIDR-format IPv4 host address.
Changed files
All integration tests
"{{ var }}"was used in asserts, change tovarplugins/modules/dcnm_vrf.py