Skip to content

Enhancing Fabric Type Support for Switches Module#405

Open
AKDRG wants to merge 6 commits into
CiscoDevNet:developfrom
AKDRG:nd_switches_enhancement
Open

Enhancing Fabric Type Support for Switches Module#405
AKDRG wants to merge 6 commits into
CiscoDevNet:developfrom
AKDRG:nd_switches_enhancement

Conversation

@AKDRG

@AKDRG AKDRG commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Proposed Changes

This PR improves nd_manage_switches support for fabric-specific switch onboarding by adding fabric capability validation, exposing supported switch platform types, and improving lifecycle output visibility for config-save and deploy operations.

Changes

  • Added fabric capability validation before switch discovery/add workflows.
  • Enforced fabric-specific support for:
  1. switch role
  2. preserve_config
  3. platform_type
  • Exposed platform_type in the playbook argspec with supported choices:
  1. nx-os
  2. ios-xe
  3. ios-xr
  4. other
  • Registered finalize operations in module output:
  1. config_save
  2. deploy_switches
  3. deploy_config
  • Preserved full controller response metadata for finalize API calls where available.
  • Added/updated unit tests for fabric validation, platform input handling, and finalize output visibility.

Test Notes

Validated against live controller
pytest tests/unit/module_utils/test_nd_switch_resources.py -q
44 passed

Cisco Nexus Dashboard Version

4.2(1)

Related ND API Resource Category

  • analyze
  • infa
  • manage
  • onemanage
  • other

Checklist

  • Latest commit is rebased from develop with merge conflicts resolved
  • New or updates to documentation has been made accordingly
  • Assigned the proper reviewers

Comment thread plugins/module_utils/manage_switches/fabric_switch_capabilities.py
@allenrobel

Copy link
Copy Markdown
Collaborator

@AKDRG — a heads-up and a question, not a request to change anything here. This PR is doing the right thing; the collision is on my side.

Situation. In #404 I added a PlatformTypeEnum to plugins/module_utils/enums.py for FabricContext.get_platform_type(), which reads additionalData.platformType off GET /fabrics/{fabric}/switches. I did that without noticing your PlatformType in models/manage_switches/enums.py already covers the same ground — same values, except yours also has SONIC, which mine is missing (a real gap: my lookup would silently return None for a SONiC switch). Two enums one token apart in the name is a trap for the next reader, so I'd rather converge than ship the duplicate.

Proposal. Promote PlatformType from models/manage_switches/enums.py up to module_utils/enums.py, and have FabricContext use it. Per CLAUDE.md, module_utils/enums.py is for "Enums commonly used by most module utilities" — with both manage_switches and fabric_context needing it, that's what it's become. The reverse (a top-level util importing from models/manage_switches/) would be a layering inversion.

This shouldn't cost you anything. Done as a move + re-export — leaving PlatformType re-exported from models/manage_switches/enums.py (via __all__, to keep pylint quiet) — every existing import site keeps working untouched, nothing in this PR changes, and you don't need to rebase. module_utils/enums.py imports only from enum import Enum, so there's no cycle, and the re-export lives in a named module rather than __init__.py, so it's clear of the empty-init sanity rule. Two of your changed files (nd_switch_resources.py, config_models.py) are PlatformType consumers, which is exactly why I'd rather not touch import sites while you're in flight.

The question I actually want your read on. Your enums look deliberately endpoint-scoped — PlatformType documented as AddSwitches (POST switches), ShallowDiscoveryPlatformType split out because shallowDiscovery excludes apic. My use is a third context: the GET switches response. Is that response set known to match the AddSwitches request set, or would you rather it stay a separate enum on principle? If they can legitimately diverge, sharing one is the wrong call and I'll keep a distinct read-side enum — properly named and documented, with SONIC added. You know this API surface better than I do.

One detail either way: normalize() defaults None -> NX_OS, which suits the write path but not mine, where "switch reports no platformType" has to stay None. So I'd use the bare enum rather than normalize() regardless.

Happy to do the move in #404 if you're good with it, or leave it to you if you'd rather own that file.

allenrobel added a commit that referenced this pull request Jul 16, 2026
get_platform_type() resolves via PlatformTypeEnum(raw) inside a try/except
ValueError, falling through to None for any value the enum does not know. With
SONIC absent, a SONiC switch silently reported "no platform type" rather than
its actual platform -- the exact failure the enum exists to prevent.

Found while checking PlatformTypeEnum against develop's pre-existing
PlatformType (plugins/module_utils/models/manage_switches/enums.py), which has
carried SONIC all along. Whether the two enums converge is an open question
being discussed with @AKDRG on #405; this gap is a defect either way, so fix it
now rather than leave it pending that outcome. If PlatformTypeEnum is later
dropped in favor of a promoted PlatformType, this member goes with it.

Test: extends 00230 with a fourth switch reporting platformType "sonic",
asserting it resolves to PlatformTypeEnum.SONIC. Verified to fail against the
unfixed enum (AttributeError: type object 'PlatformTypeEnum' has no attribute
'SONIC').

module_utils suite green: 3036 passed. black/isort/pylint/mypy clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
allenrobel added a commit that referenced this pull request Jul 19, 2026
get_platform_type() resolves via PlatformTypeEnum(raw) inside a try/except
ValueError, falling through to None for any value the enum does not know. With
SONIC absent, a SONiC switch silently reported "no platform type" rather than
its actual platform -- the exact failure the enum exists to prevent.

Found while checking PlatformTypeEnum against develop's pre-existing
PlatformType (plugins/module_utils/models/manage_switches/enums.py), which has
carried SONIC all along. Whether the two enums converge is an open question
being discussed with @AKDRG on #405; this gap is a defect either way, so fix it
now rather than leave it pending that outcome. If PlatformTypeEnum is later
dropped in favor of a promoted PlatformType, this member goes with it.

Test: extends 00230 with a fourth switch reporting platformType "sonic",
asserting it resolves to PlatformTypeEnum.SONIC. Verified to fail against the
unfixed enum (AttributeError: type object 'PlatformTypeEnum' has no attribute
'SONIC').

module_utils suite green: 3036 passed. black/isort/pylint/mypy clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa
@allenrobel

Copy link
Copy Markdown
Collaborator

@AKDRG thanks for moving PlatformType to plugins/module_utils/enums.py 👍

I'll wait for #405 to merge to leverage PlatformType at its new home.

@allenrobel allenrobel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Four findings, each as an inline comment: endpoint-import coupling in shared utils.py, role: None rejected by the new fabric-capability validation, a now-dead all_preserve_config flag, and an undocumented ValueError in normalize_platform_type.

🤖 Generated with Claude Code

Comment thread plugins/module_utils/utils.py Outdated
Comment thread plugins/module_utils/manage_switches/fabric_capabilities.py Outdated
Comment thread plugins/module_utils/manage_switches/nd_switch_resources.py
Comment thread plugins/module_utils/models/manage_switches/config_models.py
@AKDRG AKDRG added the ready for review Submitter is requesting a PR review label Jul 22, 2026
allenrobel added a commit that referenced this pull request Jul 22, 2026
get_platform_type() resolves via PlatformTypeEnum(raw) inside a try/except
ValueError, falling through to None for any value the enum does not know. With
SONIC absent, a SONiC switch silently reported "no platform type" rather than
its actual platform -- the exact failure the enum exists to prevent.

Found while checking PlatformTypeEnum against develop's pre-existing
PlatformType (plugins/module_utils/models/manage_switches/enums.py), which has
carried SONIC all along. Whether the two enums converge is an open question
being discussed with @AKDRG on #405; this gap is a defect either way, so fix it
now rather than leave it pending that outcome. If PlatformTypeEnum is later
dropped in favor of a promoted PlatformType, this member goes with it.

Test: extends 00230 with a fourth switch reporting platformType "sonic",
asserting it resolves to PlatformTypeEnum.SONIC. Verified to fail against the
unfixed enum (AttributeError: type object 'PlatformTypeEnum' has no attribute
'SONIC').

module_utils suite green: 3036 passed. black/isort/pylint/mypy clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014aJ3Y2TJEAqJZBeUKdUnGa

@allenrobel allenrobel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after comments were addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NaC-CapSet1 ready for review Submitter is requesting a PR review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants