Skip to content

fix(netbox_cable): make every termination type idempotent on re-runs - #1575

Draft
ldrozdz93 wants to merge 1 commit into
netbox-community:develfrom
ldrozdz93:int-412-cable-termination-idempotency
Draft

ldrozdz93 wants to merge 1 commit into
netbox-community:develfrom
ldrozdz93:int-412-cable-termination-idempotency

Conversation

@ldrozdz93

Copy link
Copy Markdown
Collaborator

Summary

Connecting a cable works, but re-running the same play raised an exception for every termination type except dcim.interface ↔ dcim.interface. Users had to delete cables before re-running their playbooks. This makes netbox_cable idempotent for all termination types — console, console-server, power port/outlet/feed, front/rear port and circuit termination — so a second run reports ok instead of crashing or falsely reporting a change.

What was wrong

On update, the module rebuilt each existing termination's content type from pynetbox internals (termination.endpoint.name). That reconstruction failed in three independent ways:

  • Dash vs underscore. pynetbox derives the endpoint name from the record URL in dash form (power-ports, console-ports, circuit-terminations), but the internal lookup maps are keyed in underscore form, so the lookup raised. dcim.interface was the only type that survived, because interfaces contains no dash — matching the reported "interface works, everything else fails" pattern.
  • Plain-dict terminations. For some types pynetbox returns the termination as a plain dict with no .endpoint attribute, raising AttributeError.
  • Form mismatch. Even when the lookup succeeded it produced dcim.power_port, which can never equal the user-supplied dcim.powerport, so the cable was reported as changed on every run.

A previous one-off fix had added a single dash-form key for rear ports only, papering over one endpoint at a time.

What changed

  • The termination normalizer now reads the content type directly when it is already present (plain dict or pynetbox GenericListObject), and otherwise rebuilds it from the record's endpoint, matching the dotted app.model form the module emits on the data side. This covers all termination types and is robust across pynetbox versions.
  • The rear-ports dash-form band-aid is removed, since the general path now handles every endpoint.

Tests

  • Unit tests for the normalizer covering each termination type (console, console-server, front/rear port, power port/outlet/feed, interface, circuit termination) across the three shapes pynetbox can return, plus end-to-end checks that a non-interface cable re-run reports no change while a genuine field change is still detected.
  • Integration assertions added for the second run of the console and circuit-termination cases.

Note: the netbox_cable integration target is currently disabled in the per-version main.yml (it predates the v4 task directories), so the integration assertions above are authored but not yet wired into a CI run; re-enabling and validating that target against current netbox-docker is best done as a follow-up. The unit suite is the executable regression guard for this fix.

Fixes #1274
Fixes #1015
Fixes #1040
Fixes #946
Fixes #1217

Re-running a play that connected a non-interface termination raised an
exception and could report a false `changed`. The update path rebuilt each
existing termination's content type from pynetbox internals via
`termination.endpoint.name`, which failed three ways: dash-form endpoint
names missed the underscore-keyed lookup maps, plain-dict terminations have
no `.endpoint`, and even a successful lookup produced `dcim.power_port`
instead of the user-side `dcim.powerport`.

`_convert_termination` is now a method that normalizes all three pynetbox
shapes (plain dict, GenericListObject, bare Record) to the dotted
content-type form, so cables are idempotent for console, console-server,
power port/outlet/feed, front/rear port and circuit terminations - not only
`dcim.interface`. The one-off `rear-ports` dash band-aid in
`API_APPS_ENDPOINTS` and `ENDPOINT_NAME_MAPPING` is removed.

Adds unit coverage for the reconstruction helper across all termination
types and pynetbox shapes, plus second-run integration assertions for the
console and circuit-termination cases.

Fixes netbox-community#1274
Fixes netbox-community#1015
Fixes netbox-community#1040
Fixes netbox-community#946
Fixes netbox-community#1217

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes netbox_cable idempotency for all supported termination types by normalizing existing cable terminations into the same dotted app.model form used by user-supplied module data, preventing second-run exceptions and false “changed” results.

Changes:

  • Normalize existing cable terminations across pynetbox return shapes (dict / GenericListObject-like / bare Record) via a dedicated _convert_termination() method.
  • Remove the previous special-case dash-form mapping (“rear-ports”) now that normalization handles dash vs underscore endpoints generically.
  • Add unit tests (broad termination-type coverage + idempotent re-run behavior) and add integration assertions for second-run idempotency cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
plugins/module_utils/netbox_utils.py Implements robust termination normalization and removes obsolete dash-form rear-port mapping.
tests/unit/module_utils/netbox_utils/test_netbox_module.py Adds unit coverage for termination normalization across all termination types and pynetbox shapes, plus idempotent cable re-run checks.
tests/integration/targets/v4.5/tasks/netbox_cable.yml Adds second-run idempotency assertions for console and circuit-termination cables.
changelogs/fragments/int-412-netbox-cable-termination-idempotency.yml Documents the bugfix and links impacted issues.

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

@kingspeedy95

Copy link
Copy Markdown

Independent confirmation that this fixes the problem, from a production NetBox with a large cable inventory.

Environment: NetBox 4.x, netbox.netbox 3.23.0 from devel, pynetbox 7.6.1, driven by netbox-manager, which renders one task per resource.

Before. 238 cables in the model, of which 236 are dcim.interface to dcim.interface and 2 are dcim.consoleserverport to dcim.consoleport. The two console cables were created on the first run and then failed on every subsequent run with

console-server-ports not found in API_APPS_ENDPOINTS

Because a play stops at its first failure, each of those two files lost every task below the cable as well. All 236 interface cables were unaffected, which matches the "interface works, everything else fails" pattern described here.

After, with this branch installed unchanged at 4f041ea: 0 failed plays, down from 2, across 89 resource files. Both console cable tasks report ok, not changed, so the existing cables are now recognised rather than raising, which is precisely what the PR describes.

Three observations that may be useful to a reviewer:

  • Arriving at this independently, we reached the same normalisation for the rebuild path, endpoint.name.replace("-", "_") for the lookup and .replace("_", "") on the mapped singular. Worth stressing that both halves are needed: normalising only the lookup key yields dcim.console_server_port, which can never equal the dcim.consoleserverport NetBox stores, so the cable would report changed on every run instead of raising. The changelog says this, but it is easy to miss when reviewing just the first line.
  • The plain dict and GenericListObject shapes are a real third case that a narrower fix misses. Our own local workaround only patched the rebuild path and would still have raised AttributeError on those.
  • Corroborating that this path has had little exercise: before this branch, API_APPS_ENDPOINTS["dcim"] held 35 underscored keys and exactly one hyphenated one, "rear-ports", which looks like an earlier single endpoint attempt at the same bug. It did not work either, because the following ENDPOINT_NAME_MAPPING lookup still missed. This branch removing it is the right call.

This is still marked as a draft. Is there anything blocking it, and is there anything we can usefully test? #1015 has been open since 2023-06-06, and #1040 and #1274 describe the same thing, so there is real demand.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

3 participants