FabricDeleted: address tech debt #566

allenrobel · 2025-11-26T19:39:50Z

Summary

This commit includes the following changes to FabricDeleted:

Remove the need to pre-instantiate two classes
Use latest versions of all versioned classes
Removes constraint that fabric_names list be populated

This commit includes the following changes to FabricDetails (v3)

Add fabric_names property (FabricDelete leverages this)
Add type-hints
Use proper Markdown for docstrings

Note to reviewers

Copilot has reviewed these changes.
Please merge all preceding fabric-PRs (dcnm_fabric: max line length 160 #565, FabricSummary (v2): address tech debt #564) before merging this PR
This PR is based on branch tech-debt-fabric-lint (PR dcnm_fabric: max line length 160 #565)
There may be additional commits due to dependency requirements
- ALL fabric-related classes and DcnmFabric itself have already been modified, and IT/UT passes, but I don't want to pull all of these into this PR if at all possible...

Functional changes

Instantiate the following classes internally in FabricDeleted so the user does not have to instantiate externally

FabricDetailsByName
FabricSummary

Previously, the user had to instantiate these classes and pass them to FabricDeleted.

Use latest versions of the above classes

fabric_details_v3
fabric_summary_v2

Use latest version of Resuts (results_v2)
Use latest version of FabricCommon (common_v2)

5 fabric_names.setter - do not require list to be populated

We now handle empty-list later in the code flow. This allows easier handling of result logic.

Non-functional changes

Add type-hints everywhere
Update docstrings to use Markdown
Add local copies of RestSend and Results property getter/setters
Remove pylint no-member suppression directives as they are no longer needed.
Add module docstring
Add pylint directive to suppress metaclass invalid-name

1. Unit tests for FabricSummary (v2) - This class did not have unit tests, added unit tests and fixtures - Updated dcnm_fabric/utils.py with FabricSummary (v2) support 2. FabricSummary (v2) - Added type hints throughout - Updated docstrings to use Markdown throughout - Made several public instances private (e.g. self.conversion -> self._conversion) - Updated rest_send and results properties with enhanced versions we use in other classes

No functional changes in this commit

…evice count. Use private attributes instead of property getters when calculating device count. The current code calls property getters (self.leaf_count, self.spine_count, self.border_gateway_count) which invoke verify_refresh_has_been_called(), adding unnecessary overhead. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

No functional changes in this commit. Addressing a Copilot comment regarding commented lines in fabric_summary_v2.py

…oDevNet/ansible-dcnm into tech-debt-fabric-summary-v2

Address below Copilot review comment. [nitpick] Remove redundant empty data check in all_data property. After a successful call to refresh(), self.data cannot be empty ({}) because _verify_controller_response() at line 212 would have raised a ControllerResponseError. The verify_refresh_has_been_called() call at line 318 already ensures refresh() completed successfully. This additional check is unnecessary and inconsistent with other properties like border_gateway_count, device_count, etc. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Addressing the below Copilot review comment. [nitpick] The rest_send property getter validation prevents accessing the property before calling the setter, which is overly restrictive. Internal methods like _set_fabric_summary_endpoint() (line 176) use self.rest_send.path, which would fail this check. While refresh() validates params at line 245 before calling _set_fabric_summary_endpoint(), this getter-level validation adds unnecessary coupling and prevents legitimate use cases like inspecting the rest_send object. Consider removing lines 457-460 from the getter, as the setter and refresh() method already provide adequate validation. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

There are no functional changes in this PR. 1. Lint with black after change of max line length to 160.

# Summary This commit includes the following changes to FabricDeleted: - Remove the need to pre-instantiate two classes - Use latest versions of all versioned classes - Removes constraint that fabric_names list be populated This commit includes the following changes to FabricDetails (v3) - Add fabric_names property (FabricDelete leverages this) - Add type-hints - Use proper Markdown for docstrings ## Functional changes 1. Set the following classes internally to FabricDeleted - FabricDetailsByName - FabricSummary Previously, the user had to instantiate these classes and pass them to FabricDeleted. 2. Use latest versions of the above classes - fabric_details_v3 - fabric_summary_v2 3. Use latest version of Resuts (results_v2) 4. Use latest version of FabricCommon (common_v2) 5 fabric_names.setter - do not require list to be populated We now handle empty-list later in the code flow. This allows easier handling of result logic. # Non-functional changes 1. Add type-hints everywhere 2. Update docstrings to use Markdown 3. Add local copies of RestSend and Results property getter/setters 4. Add module docstring 5. Add pylint directive to suppress __metaclass__ invalid-name

Fix below sanity error: ERROR: plugins/module_utils/fabric/delete.py:290:5: E303: too many blank lines (2)

UT asserts are failing since path and verb were redefined from None to str in FabricCommon (common_v2.py). - Override these in FabricDeleted for now. - Add TODO to remove later.

Copilot

Pull request overview

This pull request addresses technical debt in the FabricDelete class and related components by modernizing the codebase with the following key improvements:

Simplified user experience: Removes the requirement for users to pre-instantiate FabricDetailsByName and FabricSummary classes; these are now instantiated internally within FabricDelete
Version upgrades: Updates to use fabric_details_v3, fabric_summary_v2, results_v2, and common_v2
Relaxed constraints: Allows empty fabric_names lists to be set (handled gracefully in the code flow)
Code quality improvements: Adds comprehensive type hints and converts docstrings to proper Markdown format throughout
Better encapsulation: Uses private attributes with property accessors for better object-oriented design

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tests/unit/modules/dcnm/dcnm_fabric/utils.py	Adds module docstring, fixture for FabricSummaryV2, response helper function; reformats imports to single line
tests/unit/modules/dcnm/dcnm_fabric/test_verify_playbook_params.py	Reformats multi-line imports to single line
tests/unit/modules/dcnm/dcnm_fabric/test_template_get_all.py	Reformats multi-line imports to single line; unwraps assertion statements
tests/unit/modules/dcnm/dcnm_fabric/test_template_get.py	Reformats multi-line imports to single line
tests/unit/modules/dcnm/dcnm_fabric/test_ruleset.py	Reformats multi-line imports to single line; reformats multi-line assertions
tests/unit/modules/dcnm/dcnm_fabric/test_param_info.py	Reformats multi-line imports to single line
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_types.py	Reformats multi-line imports to single line; simplifies function signatures
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_summary_v2.py	New comprehensive unit test file for FabricSummary v2 with 811 lines of tests
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_details_v2.py	Reformats multi-line imports to single line; unwraps assertion statements
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_details_by_nv_pair_v2.py	Reformats multi-line imports to single line; unwraps assertion statements
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_details_by_name_v2.py	Reformats multi-line imports to single line
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_delete.py	Major refactor: updates to use fabric_details_v3, fabric_summary_v2, results_v2; removes test_fabric_delete_00032 and test_fabric_delete_00061; updates test expectations for internal instantiation of dependencies
tests/unit/modules/dcnm/dcnm_fabric/test_fabric_config_save.py	Reformats multi-line imports to single line; simplifies function signatures
tests/unit/modules/dcnm/dcnm_fabric/fixtures/responses_FabricSummary_V2.json	New fixture file with 389 lines of mock responses for FabricSummary v2 tests
plugins/module_utils/fabric_group/query.py	Updates import documentation to reference results_v2
plugins/module_utils/fabric_group/fabric_groups.py	Updates import documentation to reference results_v2
plugins/module_utils/fabric_group/fabric_group_details.py	Updates import documentation to reference results_v2
plugins/module_utils/fabric_group/create.py	Updates import documentation to reference results_v2
plugins/module_utils/fabric/template_get_all.py	Reformats multi-line import to single line
plugins/module_utils/fabric/template_get.py	Reformats multi-line import to single line
plugins/module_utils/fabric/fabric_summary_v2.py	Major update: adds comprehensive type hints, converts docstrings to Markdown format, adds property setters with type validation, uses private attributes
plugins/module_utils/fabric/fabric_details_v3.py	Major update: adds comprehensive type hints, adds fabric_names property, converts docstrings to Markdown format, uses private attributes with property accessors, removes duplicate property definitions in subclasses
plugins/module_utils/fabric/delete.py	Major refactor: adds module docstring, instantiates dependencies internally, adds comprehensive type hints, converts docstrings to Markdown, uses private attributes, adds property accessors, allows empty fabric_names list
plugins/module_utils/fabric/config_save_v2.py	Reformats multi-line import to single line
plugins/module_utils/fabric/config_save.py	Reformats multi-line import to single line

Comments suppressed due to low confidence (1)

plugins/module_utils/fabric/delete.py:299

The docstring format is inconsistent with the Markdown format used elsewhere in this file. This method uses ### Summary and ### Raises instead of # Summary and ## Raises format used in other methods like _verify_fabric_can_be_deleted, _validate_commit_parameters, commit, and _set_fabric_delete_endpoint.

        """
        ### Summary
        Send a delete request to the controller and register the result.

        ### Raises
            -   ``ValueError`` if the fabric delete endpoint cannot be set.
        """

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

plugins/module_utils/fabric/delete.py

tests/unit/modules/dcnm/dcnm_fabric/test_fabric_details_v2.py

tests/unit/modules/dcnm/dcnm_fabric/test_fabric_summary_v2.py

tests/unit/modules/dcnm/dcnm_fabric/test_fabric_types.py

1. Fix inconsistent use of self._rest_send vs self.rest_send 2. _send_requests() add method signature return type annotation 3. Update remaining docstrings to proper Markdown

Fix the below sanity errors: ERROR: plugins/module_utils/fabric/delete.py:107:1: W293: blank line contains whitespace ERROR: plugins/module_utils/fabric/delete.py:343:1: W293: blank line contains whitespace

Address the following error in sanity tests: ERROR: tests/unit/modules/dcnm/dcnm_fabric/utils.py:20:0: misplaced-future: __future__ import is not the first non docstring statement

allenrobel and others added 10 commits November 24, 2025 09:56

Appease linters

2f9344c

No functional changes in this commit

FabricSummary (v2): Explain why lines are commented

ebfa9f0

No functional changes in this commit. Addressing a Copilot comment regarding commented lines in fabric_summary_v2.py

Merge branch 'tech-debt-fabric-summary-v2' of https://github.com/Cisc…

39c0640

…oDevNet/ansible-dcnm into tech-debt-fabric-summary-v2

dcnm_fabric: black 160 character line length

f9cce22

There are no functional changes in this PR. 1. Lint with black after change of max line length to 160.

Merge branch 'tech-debt-fabric-lint' into tech-debt-fabric-deleted

deff137

allenrobel self-assigned this Nov 26, 2025

allenrobel added the Work in Progress Code not ready for review. label Nov 26, 2025

allenrobel added 2 commits November 26, 2025 09:45

Appease linters

224aeb0

Fix below sanity error: ERROR: plugins/module_utils/fabric/delete.py:290:5: E303: too many blank lines (2)

FabricDeleted: temporarily override path and verb

c19e15c

UT asserts are failing since path and verb were redefined from None to str in FabricCommon (common_v2.py). - Override these in FabricDeleted for now. - Add TODO to remove later.

allenrobel requested a review from Copilot November 26, 2025 19:58

Copilot started reviewing on behalf of allenrobel November 26, 2025 19:58 View session

Copilot finished reviewing on behalf of allenrobel November 26, 2025 20:00

Copilot AI reviewed Nov 26, 2025

View reviewed changes

Address Copilot review comments

63e654b

1. Fix inconsistent use of self._rest_send vs self.rest_send 2. _send_requests() add method signature return type annotation 3. Update remaining docstrings to proper Markdown

allenrobel added ready for review PR is ready to be reviewed Work in Progress Code not ready for review. and removed Work in Progress Code not ready for review. ready for review PR is ready to be reviewed labels Nov 26, 2025

Appease linters

4597931

Fix the below sanity errors: ERROR: plugins/module_utils/fabric/delete.py:107:1: W293: blank line contains whitespace ERROR: plugins/module_utils/fabric/delete.py:343:1: W293: blank line contains whitespace

allenrobel added ready for review PR is ready to be reviewed and removed Work in Progress Code not ready for review. labels Nov 26, 2025

allenrobel requested review from mikewiebe and mtarking December 2, 2025 20:49

allenrobel added 2 commits December 3, 2025 10:20

Merge branch 'develop' into tech-debt-fabric-lint

dbb8734

Merge branch 'tech-debt-fabric-lint' into tech-debt-fabric-deleted

67d3145

allenrobel added 2 commits December 3, 2025 10:31

Address Sanity error (pylint)

b895a00

Address the following error in sanity tests: ERROR: tests/unit/modules/dcnm/dcnm_fabric/utils.py:20:0: misplaced-future: __future__ import is not the first non docstring statement

Merge branch 'tech-debt-fabric-lint' into tech-debt-fabric-deleted

1a2efcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FabricDeleted: address tech debt #566

FabricDeleted: address tech debt #566

Uh oh!

allenrobel commented Nov 26, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FabricDeleted: address tech debt #566

Are you sure you want to change the base?

FabricDeleted: address tech debt #566

Uh oh!

Conversation

allenrobel commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note to reviewers

Functional changes

Non-functional changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

allenrobel commented Nov 26, 2025 •

edited

Loading