Simplify netmiko_send_commands method for clarity and maintainability #453
Conversation
mzbroch
requested review from
Dav-C,
glennmatthews,
jeffkala and
scetron
as code owners
mzbroch
changed the title
jeffkala
left a comment
jeffkala
left a comment
There was a problem hiding this comment.
Looks good. Relying on the test quite a bit for the logic not changing but went through it slowly and I think this is a much cleaner and manage code base.
Just a few questions but otherwise I"m good with it.
| if validation_result: | ||
| return validation_result | ||
|
|
||
| if connectivity_test and not _check_connectivity(task): |
There was a problem hiding this comment.
FWIW, this is also already in nornir-nautobot as a dispatcher function. I understand this is step 1 of this revamp, but keeping here for awareness for future.
There was a problem hiding this comment.
Yes — I needed to simplify it initially so that I could more easily migrate to nornir-nautobot.
|
|
||
| def _handle_netmiko_error(task, exception): | ||
| """Handle connection/authentication errors gracefully.""" | ||
| from netmiko.exceptions import NetmikoTimeoutException, NetmikoAuthenticationException |
There was a problem hiding this comment.
Yes, that's incorrect here.
| skip_list = [ | ||
| "cables", | ||
| "interfaces__tagged_vlans", | ||
| "interfaces__untagged_vlan", | ||
| "interfaces__vrf", | ||
| "software_version", | ||
| "vlan_map", | ||
| ], |
There was a problem hiding this comment.
I'm confused by this, mind explaining this a bit more?
| skip_list = [ | ||
| "cables", | ||
| "interfaces__tagged_vlans", | ||
| "interfaces__untagged_vlan", | ||
| "software_version", | ||
| "vlan_map", | ||
| ] |
There was a problem hiding this comment.
Assuming the explaination from the previous comment, maybe we should move this somehow into constants.py
| ) | ||
| }, | ||
| defaults=Defaults(data={"sync_vlans": False, "sync_vrfs": False, "sync_cables": False}), | ||
| # defaults=Defaults(data={"sync_vlans": False, "sync_vrfs": False, "sync_cables": False}), |
Summary
This PR is
Stage 1refactor before moving components out of onboarding app to nornir-nautobot.PR refactors the
netmiko_send_commandsfunction to remove duplicated logic, improve readability, and make command filtering more maintainable — without changing any behavior..Key Improvements
✅ Removed redundant loops and conditions
Consolidated repeated logic for handling commands as lists or dicts.
✅ Centralized skip logic
Introduced a single
command_exclusionsmapping instead of multiple scattered if not sync_* checks.✅ Decoupled jobs from automation runner
Removed job and job inputs dependency in lower level methods, separating them from actual user input
✅ Improved readability
Clearer handling of the "pre_processor" section and command collection flow.
✅ Behavior preserved
All original skip rules, deduplication behavior, and flag-based filters remain identical.
✅ Safer and cleaner
Uses get("commands", []) and .extend() for simpler and more resilient logic.