dcnm_fabric: Remove FabricSummary v1 & fix query-state #567

allenrobel · 2025-11-26T21:52:03Z

Summary

All dependencies on FabricSummary v1 have been (hopefully) moved to FabricSummary v2.

This PR includes the following major changes:

Remove FabricSummary v1.
Fix query-state handling in dcnm_fabric.py (this was found with IT dcnm_fabric_query_state.yaml)

Notes to reviewers

We needed to commit additional changes to other libraries due to the dependency matrix (else Github UT fails)
- Local UT passes, but this is because we have modified about 20 other files. I have committed all of these now and Github UT is now passing.
We have run Copilot review and addressed its comments.
The following basic IT tests are passing (I'll run the others on 2025-11-28)
- dcnm_fabric_deleted_basic.yaml
- dcnm_fabric_deleted_basic_lan_classic
- dcnm_fabric_deleted_basic_msd
- dcnm_fabric_deleted_basic_vxlan
- dcnm_fabric_merged_basic.yaml
- dcnm_fabric_replaced_basic
- dcnm_fabric_query_basic
All preceding "tech debt" PRs should be merged before merging this PR.

Changes

Remove plugins/module_utils/fabric/fabric_summary.py
tests/unit/modules/dcnm/dcnm_fabric/utils.py

Remove references and imports for FabricSummary v1

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

Remove unit tests for FabricSummary v1

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

Remove unit tests for FabricCommon v1 since these import FabricSummary v1

dcnm_fabric.py

Fix query-state params handling. In Common.populate_config() add an else clause that handles copying params into query-state self.config.

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.

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

# Summary All dependencies on FabricSummary v1 have been (hopefully) moved to FabricSummary v2. This PR removes FabricSummary v1. ## Changes 1. Remove plugins/module_utils/fabric/fabric_summary.py 2. tests/unit/modules/dcnm/dcnm_fabric/utils.py - Remove references and imports for FabricSummary v1 3. tests/unit/modules/dcnm/dcnm_fabric/test_fabric_summary.py - Remove unit tests for FabricSummary v1 4. tests/unit/modules/dcnm/dcnm_fabric/test_common.py Remove unit tests for FabricCommon v1 since these import FabricSummary v1

Sorry, but the dependencies are across all (or most) of these files, so these cannot be commited independently. In the future, I plan to lint first as a separate PR to reduce the “noise” but, alas, I didn’t think to do that this time.

No functional changes in this commit.

Copilot

Pull request overview

This PR removes FabricSummary v1 and migrates all dependencies to FabricSummary v2, as part of ongoing technical debt reduction. The changes successfully eliminate the deprecated v1 implementation while updating tests, fixtures, and module code to use the v2 API.

Key Changes

Removed plugins/module_utils/fabric/fabric_summary.py (v1 implementation)
Updated all imports from fabric_summary to fabric_summary_v2
Updated test fixtures and utilities to reference v2 classes and response files
Added new fixture files: responses_FabricSummary_V2.json and payloads_FabricCommon_V2.json
Updated dcnm_fabric.py module to import and use v2 implementations
Applied code formatting improvements (multi-line imports consolidated to single lines)

Reviewed changes

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

Show a summary per file

File	Description
`plugins/module_utils/fabric/fabric_summary.py`	Removed v1 implementation (363 lines deleted)
`tests/unit/modules/dcnm/dcnm_fabric/utils.py`	Updated imports and fixtures to use v2 classes; added type hints
`tests/unit/modules/dcnm/dcnm_fabric/test_*.py`	Updated imports to single-line format; updated fixture references to v2
`plugins/modules/dcnm_fabric.py`	Updated to import FabricSummary from fabric_summary_v2; added type hints
`plugins/module_utils/fabric/update.py`	Updated imports and internal references to use v2 classes
`plugins/module_utils/fabric/replaced.py`	Updated imports to use common_v2 and fabric_summary_v2
`plugins/module_utils/fabric/query.py`	Updated imports to use common_v2 and results_v2
`plugins/module_utils/fabric_group/*.py`	Updated imports to use results_v2
`tests/unit/modules/dcnm/dcnm_fabric/fixtures/responses_FabricSummary_V2.json`	Added v2 response fixtures (389 lines)
`tests/unit/modules/dcnm/dcnm_fabric/fixtures/payloads_FabricCommon_V2.json`	Added v2 payload fixtures (40 lines)

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

plugins/module_utils/fabric/config_deploy.py

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

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

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

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

1. plugins/module_utils/config_deploy.py - Remove commented code 2. tests/unit/modules/dcnm/dcnm_fabric/test_fabric_replaced_bulk.py - Remove commented test cases that are no longer needed 3. tests/unit/modules/dcnm/dcnm_fabric/test_fabric_create_common.py - Unused import of 'EpFabricCreate' removed

Align @rest_send setter with latest enhancements by adding a check to verify value.params is set.

Wait until commit() to set: FabricDetailsByName.rest_send FabricDetailsByName.results Since rest_send and results are not fully initialized until commit() is called.

# Summary Fix issue where query-state configuration is ignored. Not sure why this ever worked? I think we initially wanted an empty query config to return all fabrics. Maybe we still want this; in which case we need to modify query.py accordingly. Anyway, for now, we populate self.config for query-state if params is not None. An alternative fix would be to mandate that params is not empty and add query-state to the list of states that requires a config. Currently, in query.py, a config is required so an empty config will result in an error here. This fix at least ensures that a populated query config is accepted in dcnm_fabric.py and passed downstream to query.py.

Copilot

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

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

plugins/module_utils/fabric/update.py

No functional changes in this commit.

…c-summary-v1

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

…c-summary-v1

allenrobel and others added 15 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

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.

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

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 self-assigned this Nov 26, 2025

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

allenrobel added 4 commits November 27, 2025 10:38

Add remaining files

497e8e5

Sorry, but the dependencies are across all (or most) of these files, so these cannot be commited independently. In the future, I plan to lint first as a separate PR to reduce the “noise” but, alas, I didn’t think to do that this time.

Remove debug print() statements

28a1eb6

Add module docstring

39693f0

Appease linters

59d8417

No functional changes in this commit.

allenrobel requested a review from Copilot November 27, 2025 21:17

Copilot started reviewing on behalf of allenrobel November 27, 2025 21:17 View session

Copilot finished reviewing on behalf of allenrobel November 27, 2025 21:18

Copilot AI reviewed Nov 27, 2025

View reviewed changes

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

allenrobel added 2 commits November 27, 2025 13:18

FabricCreateCommon: RestSend.setter; Add check for value.params

8412135

Align @rest_send setter with latest enhancements by adding a check to verify value.params is set.

RestSend.setter; Add check for value.params

ce6f532

Align @rest_send setter with latest enhancements by adding a check to verify value.params is set.

allenrobel added 2 commits November 27, 2025 13:34

FabricQuery.__init__(): don’t set FabricDetailsByName

d703426

Wait until commit() to set: FabricDetailsByName.rest_send FabricDetailsByName.results Since rest_send and results are not fully initialized until commit() is called.

allenrobel changed the title ~~dcnm_fabric: Remove FabricSummary v1~~ dcnm_fabric: Remove FabricSummary v1 & fix query-state Nov 30, 2025

allenrobel requested a review from Copilot November 30, 2025 21:56

Copilot started reviewing on behalf of allenrobel November 30, 2025 21:57 View session

Copilot finished reviewing on behalf of allenrobel November 30, 2025 21:57

Copilot AI reviewed Nov 30, 2025

View reviewed changes

plugins/module_utils/fabric/update.py Outdated Show resolved Hide resolved

Fix typo in docstring (address Copilot comment)

d3ea42b

No functional changes in this commit.

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

allenrobel added 6 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

Merge branch 'tech-debt-fabric-deleted' into dcnm-fabric-remove-fabri…

df9fbd4

…c-summary-v1

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

Merge branch 'tech-debt-fabric-deleted' into dcnm-fabric-remove-fabri…

7e1fbc9

…c-summary-v1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

dcnm_fabric: Remove FabricSummary v1 & fix query-state #567

dcnm_fabric: Remove FabricSummary v1 & fix query-state #567

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!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dcnm_fabric: Remove FabricSummary v1 & fix query-state #567

Are you sure you want to change the base?

dcnm_fabric: Remove FabricSummary v1 & fix query-state #567

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

Notes to reviewers

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

allenrobel commented Nov 26, 2025 •

edited

Loading