Add hp_procurve_show_interfaces_status#2312
Conversation
|
@julmanglano Thoughts? |
matt852
left a comment
There was a problem hiding this comment.
One suggestion, along with @mjbear's comment:
Drop the ? on the inter-column whitespace. Non-greedy \s+? between two \S-style captures is semantically identical to \s+, and \s+ is the dominant convention across the repo's show_interfaces_status templates. In the Ports state:
- ^\s+${PORT}\s+?${NAME}\s+${STATUS}\s+?${CONFIG_MODE}\s+?${SPEED}\s+?${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
- ^\s+${PORT}\s+?${NAME}\s+${STATUS}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
- ^\s+${PORT}\s+${STATUS}\s+?${CONFIG_MODE}\s+?${SPEED}\s+?${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
- ^\s+${PORT}\s+${STATUS}\s+?${CONFIG_MODE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
+ ^\s+${PORT}\s+${NAME}\s+${STATUS}\s+${CONFIG_MODE}\s+${SPEED}\s+${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
+ ^\s+${PORT}\s+${NAME}\s+${STATUS}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
+ ^\s+${PORT}\s+${STATUS}\s+${CONFIG_MODE}\s+${SPEED}\s+${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
+ ^\s+${PORT}\s+${STATUS}\s+${CONFIG_MODE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record|
Thanks @mjbear — the truncation in For trunk membership, So the tolerance is keeping data that lives only in this command. Worth keeping unless you see a downside? |
…s+? with \s+ Per matt852's review: between two \S-style captures, non-greedy and greedy whitespace are semantically identical (backtracking forces the same boundaries either way), and \s+ is the dominant convention in the repo's other show_interfaces_status templates. Both fixtures continue to pass — no behavior change.
|
Thanks @matt852 — applied. Replaced |
|
If this command provides output others don't then it's fine to keep it. I removed a bunch of duplication to make the newly added test data succinct. |
|
Thanks @mjbear — pulled the clean up, tests still green. |
|
I had one last item that caught my eye. For this simple output we could have all the rules in the Start state. The benefit is less duplicate patterns in the template. (Note: I made these changes.) |
|
Thanks @mjbear — pulled the consolidation, tests still green. |
matt852
left a comment
There was a problem hiding this comment.
One additional optional finding from the AI agent review for possibly dropping a couple lines that show as unused.
Two row-shape rules in the Start state aren't exercised by either fixture. Specifically the rules at lines 13 and 15 of hp_procurve_show_interfaces_status.textfsm:
In ntc_templates/templates/hp_procurve_show_interfaces_status.textfsm, the Start state currently includes:
^\s+${PORT}\s+${NAME}\s+${STATUS}\s+${CONFIG_MODE}\s+${SPEED}\s+${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
- ^\s+${PORT}\s+${NAME}\s+${STATUS}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
^\s+${PORT}\s+${STATUS}\s+${CONFIG_MODE}\s+${SPEED}\s+${TYPE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
- ^\s+${PORT}\s+${STATUS}\s+${CONFIG_MODE}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> Record
^\s+${PORT}\s+${STATUS}\s+${VLAN_TAGGED}\s+${VLAN_UNTAGGED}\s*$$ -> RecordI confirmed this by deleting both rules and re-running the scoped tests — all 5 still passed with identical parsed output, so neither rule fires against the current .raw fixtures.
Recommended fix:
- Option A — drop the two unused rules (the diff above). Validated locally; parsed output is unchanged.
- Option B — keep both rules and add one fixture row each that exercises them. Your PR body names both variants ("name + tagged/untagged, no config/speed/type — e.g. trunks" and "status + config + tagged/untagged, no Name/Speed/Type"), so if you've seen them in real device output, dropping a representative line into one of the
.rawfiles and regenerating the matching.yml(poetry run python cli.py gen-yaml-folder -f tests/hp_procurve/show_interfaces_status) would cover them.
Why
The project standard is that permissive constructs and fallback rules must be justified by test data exercising every branch — anything that can't be triggered by a fixture either needs the fixture or needs to be tightened. Option A is the safer cleanup if those variants haven't been seen in the wild; Option B preserves your stated intent if they have.Thanks!
…apes with selective column population
|
Thanks @matt852 — Probed on hp_procurve devices and both shapes turned up on real device output:
So both rules represent real device behaviour. Went with Option B and added two fixtures sanitised from the captures:
Yml regenerated for both via textfsm; line 13 fires on Pushed. |
ISSUE TYPE
COMPONENT
hp_procurve,
show interfaces statusSUMMARY
Adds a new template for the HP/Aruba ProCurve
show interfaces statuscommand. The output is a fixed-width table with columns Port / Name /
Status / Config-mode / Speed / Type / Tagged / Untagged.
The Ports state holds five row-shape rules to handle the variants the
device emits:
Two fixtures:
show_interfaces_status.{raw,yml}— standalone switch with portnames like
A1,A8and truncated names (HP's 10-charNamecolumncuts off long descriptions with
...).show_interfaces_status2.{raw,yml}— 84-row stacked switch withports like
1/1, plus trunked-port names that get truncated mid-nameto the form
1/28-...(the 8-charPortcolumn truncating1/28-Trk1).The PORT regex is
[a-zA-Z0-9\-/.]+—/for stacked port format,.for truncation indicator.Cross-vendor capture-group naming aligned with existing
*_show_interfaces_statustemplates (PORT, NAME, STATUS, CONFIG_MODE,SPEED, TYPE, VLAN_TAGGED, VLAN_UNTAGGED).
Index entry inserted at the correct length-sorted position within the
hp_procurveblock.