-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW] - Adds test for switch config types #784
base: master
Are you sure you want to change the base?
Conversation
Result of running script to remove backslashes ```bash find . -type f -exec sed -i ':a;N;$!ba;s/\\\n\s*//g' {} \; ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for push this up @cjmakes ! Ill review it more in depth when I have some time later this week, but lets approve it so we can see how CI runs
also tagging @owendelong since this changes the formatting of the switchtypes files slightly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjmakes this looks really good! Left my comments inline so let me know what you think
meta = { | ||
"file": directory + filename, | ||
"header": False, | ||
"count": "1+", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the count here should be 4+
since it looks like we have a mix of 4-6
columns for the config types
"header": False, | ||
"count": "1+", | ||
"cols": [ | ||
ds.isvalidport, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having isvalidport
and isvalidlink
is definitely a great start to testing these switch config types, nice work!
exSigns,exVmVendor,vendor_backbone - Uplink | ||
TRUNK ge-0/0/1 exSCALE-SLOW,exSCALE-FAST,exSpeaker,exInfra,exAVLAN, \ | ||
exSigns,exVmVendor,vendor_backbone - Downlink | ||
TRUNK ge-0/0/0 exSCALE-SLOW,exSCALE-FAST,exSpeaker,exInfra,exAVLAN, exSigns,exVmVendor,vendor_backbone - Uplink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id prefer it we update test_datafile
to support the \
for multiline entries. Would you mind seeing how feasible that approach would be?
Thanks for fixing the linting errors that came up in CI as well 👍 |
Description of PR
Previous Behavior
There was no automation to enforce consistency across switch config types.
Tests
This pr adds tests. I would appreciate feed back on how to make sure I didn't break any automation that parses the switch configs and relies on using \ to split a config across lines.
closes #676