Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

cjmakes
Copy link

@cjmakes cjmakes commented Oct 1, 2024

Description of PR

  • Formats switch-config/type so that each config is a single line
  • Adds a datasources test for switchconfig types

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

Copy link
Member

@sarcasticadmin sarcasticadmin left a 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

@sarcasticadmin sarcasticadmin changed the title Adds test for switch config types [REVIEW] - Adds test for switch config types Oct 2, 2024
Copy link
Member

@sarcasticadmin sarcasticadmin left a 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+",
Copy link
Member

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,
Copy link
Member

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
Copy link
Member

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?

@sarcasticadmin
Copy link
Member

Thanks for fixing the linting errors that came up in CI as well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

init datasource test for switch-config types
2 participants