[ND 4.2+] Add modules for both Routing Policies Community Lists and Extended Community Lists (NDA-28) - #287
[ND 4.2+] Add modules for both Routing Policies Community Lists and Extended Community Lists (NDA-28)#287gmicol wants to merge 19 commits into
Conversation
gmicol
added
enhancement
github-actions
Bot
changed the title
dcne-automation
changed the title
6204e42 to
a272206
Compare
a272206 to
8fd0005
Compare
…xtended Community Lists.
Use PEP 585/604 annotations throughout the new module stacks and remove obsolete Python 2 boilerplate from production and unit-test files.
Normalize tenant-qualified names to bare Ansible config names while using computed API names for identifiers, payloads, update paths, and bulk deletes. Enforce the default and fully qualified API name length limits and cover same-name lists across tenants.
Use Pydantic validation context to enforce type-specific entry rules only for Ansible config. ND response parsing now tolerates controller-added cross-type fields while config validation remains strict.
Require type and entries during merged, replaced, and overridden validation using the state context supplied by NDStateMachine. Identifier-only models remain valid for deleted state and response parsing.
Normalize controller-added false flags and empty collections out of diff comparisons so identical merged configuration is stable. Explicitly clearing an active flag still produces an update.
Expose the OpenAPI clusterName query parameter as cluster_name and compose it with shared Lucene filter, pagination, and sort parameters. Add cluster_name to both module argument specs and cover item and collection URLs.
Pass explicit create, update, and delete operation types on custom bulk and item mutation requests. Results and verbose API history now classify writes correctly instead of recording them as queries.
Walk collection endpoints with bounded max and offset parameters, de-duplicate repeated rows, and stop safely when a controller ignores offset. Forward cluster_name to item, collection, and mutation endpoints through the shared endpoint configuration hook.
Use the NDStateMachine preflight hook for fabric mutation validation so check mode still fails early on unsafe writes. Collection reads and delete reconciliation no longer invoke mutation-only validation.
Treat only explicit failed, failure, and error tokens as per-item bulk failures, matching the current route-map denylist behavior. Accepted, empty, missing, and future informational status values no longer cause spurious module failures.
Set version_added to 2.0.0, remove the unsupported description example, and document cluster and tenant-qualified behavior. Define the standard state-machine return fields and representative samples for both modules.
Verify merged idempotency in both check and normal modes. Add opt-in coverage for same-name objects in two tenants, guarded by nd_test_tenant_name_a and nd_test_tenant_name_b, with tenant-aware cleanup.
Remove obsolete Python 2 future imports from the shared endpoint mixin file touched by the new community-list name mixins.
Add the class-level spacing required by pycodestyle. This resolves the sole failure reported by the containerized sanity run.
Require every standard community-list entry to select a community number or enable a well-known flag for config writes. Keep controller response parsing permissive, correct the module documentation, and add unit plus integration regression definitions.
Accept the comma-packed route-target grammar allowed by OpenAPI and normalize it to canonical list items for config and response data. Document the behavior and cover serialization, response parsing, invalid input, and idempotency.
b7dd305 to
83c2c89
Compare
allenrobel
left a comment
allenrobel
left a comment
There was a problem hiding this comment.
Code review (follow-up)
Re-reviewed the commits pushed since my last review. All ten review-response fixes check out as claimed — nice work, especially the tenant-scoped api_name identity and the diff-normalization/idempotency mechanics, which I traced end to end. Three additive findings below as inline comments.
🤖 Generated with Claude Code
| data["name"] = self.api_name | ||
| return data | ||
|
|
||
| def to_diff_dict(self, **kwargs) -> dict[str, Any]: |
There was a problem hiding this comment.
This to_diff_dict() override (and its twin in manage_extended_community_list.py line 298) works around an ND wire-shape discrepancy: ND echoes false well-known-flag defaults (and empty collections, in the extended model) for fields the user never set. Per the team's workaround-marker convention, each such site needs its own TODO(4.2.1) <slug> marker so the workaround can be audited and removed later — same pattern as the route-map model's next-hop boolean stripping in manage_route_map.py. Until we've found a way to share the bug-tracker data that backs the slugs, please use TBD as the slug (as the route-map PR did), e.g.:
# TODO(4.2.1) TBD
# ND echoes false/empty defaults for unset standard-entry fields on GET; strip them so diffs stay idempotent.One marker here and one on the extended twin.
| if getattr(entry, field) is not None: | ||
| raise ValueError(f"entries[{i}].{field} must not be set for " "type='expanded'. Use community_number_regex instead.") | ||
| else: | ||
| # standard |
There was a problem hiding this comment.
Follow-up to "Reject flagless standard community entries": that commit added an at-least-one-selector rule to CommunityListModel (a standard entry must set at least one community number or well-known flag), but the standard branch here never got the equivalent. All five selector collections default to None, so a standard entry with only sequence_number + action passes local validation and is sent to ND — the same failure mode Mike lab-verified for community lists. The module docs also still say entries "may set any combination of" the collections, with no at-least-one requirement.
Suggest mirroring the community-list fix: for config input, require at least one of the five collections to be non-empty, update the DOCUMENTATION/notes, and add the matching unit coverage. (Caveat: I have not lab-verified that ND rejects the extended variant the way it rejects flagless standard community entries — worth a quick check while you're in there.)
|
|
||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| cluster_name: str | None = Field( |
There was a problem hiding this comment.
cluster_name is hand-declared here (and in manage_extended_community_lists.py line 47) with a definition identical to the existing ClusterNameMixin in endpoints/mixins.py, whose stated purpose is adding common fields to endpoint models without duplication. The other endpoint-params classes in the collection compose the mixin for this exact field, and model_config = ConfigDict(extra="forbid") layers fine on top of it elsewhere. Suggest class CommunityListsEndpointParams(ClusterNameMixin, EndpointQueryParams) (and the twin) and dropping the local field.
6 participants
Related Issue(s)
Fixes #241
Fixes #242
Proposed Changes
nd_manage_community_listfor ND 4.2+ Routing Policies Community Lists.nd_manage_extended_community_listfor ND 4.2+ Routing Policies Extended Community Lists.NDStateMachine.merged,replaced,overridden, anddeletedstates.Test Notes
30 passed.Cisco Nexus Dashboard Version
Developed against Cisco Nexus Dashboard 4.2.1 / ND 4.2+.
Related ND API Resource Category
Checklist