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

Ipconfig bare i63 #78

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Ipconfig bare i63 #78

merged 4 commits into from
Jun 16, 2021

Conversation

cmwill
Copy link
Collaborator

@cmwill cmwill commented May 14, 2021

Related to issue #63
Adds support for parsing the ipconfig command without /all or /all /allcompartments

@marshall-sg marshall-sg linked an issue May 25, 2021 that may be closed by this pull request
@marshall-sg marshall-sg added this to To do in FC#001 via automation May 25, 2021
@marshall-sg marshall-sg moved this from To do to WIP: Week in FC#001 May 25, 2021
Copy link
Collaborator

@marshall-sg marshall-sg left a comment

Choose a reason for hiding this comment

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

See my comments on the code, but in addition to that:

  • There is also a section which you didn't change that is impacted. The new sample configs don't have a Host Name field as typical when of our prior examples. Thus the logic of the DNS Suffix is wrong as it attempts to prepend an empty hostname since the parser normally would have a value for it.
  • You probably want to create a unit test for the parser to test the logic. You can see a sample at datastore/importers/nmdb-import-ping/Parser.test.cpp. I can also create it for you if want, but it will take me a bit to get time for it.

@marshall-sg marshall-sg moved this from WIP: Week to WIP: Fortnight in FC#001 May 25, 2021
…works

Fix for error when parsing default gateways
@cmwill
Copy link
Collaborator Author

cmwill commented May 28, 2021

I think that I've fixed it such that it can parse ipconfig output without failing, though I still need to go back and account for the changes in fields when parsing bare ipconfig vs ipconfig /all.

FC#001 automation moved this from WIP: Fortnight to WIP: Week Jun 1, 2021
addIfaceIp will no longer attempt to prepend hostnames for bare ipconfig output
@cmwill
Copy link
Collaborator Author

cmwill commented Jun 9, 2021

I made some fixes/changes based on your suggestions, though I haven't made any test cases yet as I would need to make the parser rules protected rather than private, and I would rather save that for last. I think this does cover all but the tests.

As for tests, I was thinking of covering these sorts of cases. Do you have any ideas for others that should be covered?

  1. Bare
  2. /allcompartments /all
  3. /all
  4. An adapter interface type with multiple words (the Wireless LAN interface in previous test cases caused trouble, and adapters with multiword types didn't seem to be parsed correctly)
  5. The last adapter has 0 IPs in its Default Gateway
  6. The last adapter has 1 IP in its Default Gateway
  7. The last adapter has 2+ IPs in its Default Gateway

@marshall-sg
Copy link
Collaborator

I made some fixes/changes based on your suggestions, though I haven't made any test cases yet as I would need to make the parser rules protected rather than private, and I would rather save that for last. I think this does cover all but the tests.

As for tests, I was thinking of covering these sorts of cases. Do you have any ideas for others that should be covered?

  1. Bare
  2. /allcompartments /all
  3. /all
  4. An adapter interface type with multiple words (the Wireless LAN interface in previous test cases caused trouble, and adapters with multiword types didn't seem to be parsed correctly)
  5. The last adapter has 0 IPs in its Default Gateway
  6. The last adapter has 1 IP in its Default Gateway
  7. The last adapter has 2+ IPs in its Default Gateway

I'll prefix: us adding test cases is "new" and we've been doing this as we modify code, so why I made mention of it and why you see there aren't a lot existing.

Generally speaking, what we've been doing is having tests which cover the parts of the parser (this covers probably 4-7 of your list). So they are more unit test oriented. Definitely have "good cases" (they pass) and possibly "bad cases" (they fail). The good cases are basically what the rule is codified to parse. In some cases, the rules have an attached predicate and you can also examine/test the state of the object after a particular parse path. If possible, have a few bad cases which cover instance we explicitly are aware of and want to prevent a refactor from allowing. The parser is pretty good at failing on "bad cases" because of formatting on it's own.

If reasonable, a few interesting whole parser test cases (this covers probably 1-3 of your list) could be added to help a developer understand the general overall structure of the data and some interesting corner cases. However, in many cases it is not reasonable to codify (e.g., a router/switch config) so it's a judgement call. We do run changes through a separate, independent, regression testing so it is covered somewhat there as well.

That said, for parts, probably test:

  • compartmentHeader
    • Good case(s): an arbitrary line of text preceded and followed by a line with only one, or more, equal signs
    • Bad case(s): multiple lines in the center, doesn't start/end with a equal sign line, any other character besides an equal or ending newline in the pre/post-line
  • hostData
    • Good case(s): tests of host name and dns suffix
    • Bad case(s): ?
  • adapter
    • Good case(s): your 5-7, but also all the other parts, except servers since it has it's own test, of the or block
    • Bad case(s): ?
  • ifaceTypeName
    • Good case(s): single and multi-word name
    • Bad case(s): ?
  • dots
    • Good case(s): just a colon, a dot-space colon, and a couple dot-space instances then a colon
    • Bad case(s): ?
  • servers
    • Good case(s): all the parts of the or block
    • Bad case(s): ?
  • ipLine
    • Good case(s): three variations on address prefix, test with and without Subnet Mask line afterwards
    • Bad case(s): ?
  • getIp
    • Good case(s): an IP with zero or one tokens after
      • note: that the IP address parser already tests IP formats so don't worry about that
    • Bad case(s): an IP with multiple tokens after
  • ifaceType
    • Good case(s): (your 4) single and multi-word type, one or more spaces before adapter
    • Bad case(s): something without adapter after

Don't worry about testing token or ignoredLine as they are fairly simple and we will ultimately, probably, refactor those to a common Parser logic include for consistency across parsers.

@marshall-sg marshall-sg mentioned this pull request Jun 16, 2021
@marshall-sg marshall-sg merged commit bf72a2b into netmeld:master Jun 16, 2021
FC#001 automation moved this from WIP: Week to Complete PRs Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
FC#001
  
Complete PRs
Development

Successfully merging this pull request may close these issues.

Add support for bare command to nmdb-import-ipconfig
2 participants