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

Added AOS Show Commands #701

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

Conversation

Alexpf20210007
Copy link

Description

  • AOS
    • Added ShowInterfacesBrief:
      • show interfaces brief
    • Added ShowVlan:
      • show vlan
    • Added ShowInterfacesTransceiver:
      • show interfaces transceiver
    • Added ShowStacking:
      • show stacking

Motivation and Context

The commands for Aruba OS are different than Cisco IOS, IOSXE, IOSXR, etc.

Checklist:

  • [X ] I have updated the changelog.
  • I have updated the documentation (If applicable).
  • [X ] I have added tests to cover my changes (If applicable).
  • All new and existing tests passed. - I have not tested the test files.
  • All new code passed compilation.

@Alexpf20210007 Alexpf20210007 requested a review from a team as a code owner September 5, 2022 15:57
@Alexpf20210007
Copy link
Author

I recently got Aruba OS (aos) merged into the unicon plugin. This opens Aruba OS to anybody that wants to contribute to the genieparser project. I have already created 4 show commands to get started with.

@Alexpf20210007
Copy link
Author

I have an SDK folder with sample output results. Please let me know if I need to remove that.

Copy link
Contributor

@GerriorL GerriorL left a comment

Choose a reason for hiding this comment

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

Hi Alex! Thank you for this submission, and for the initiative in getting AOS supported. I do have some feedback.

Firstly, there is no need to create a new file for each parser. Parsers that operate off the same or closely related parent commands can be added to the same file. For examples all of the 'show interfaces' commands could be added to a single 'show_interfaces.py'; there is no need to have interfaces brief and interfaces transceiver listed separately.

Next, when it comes to writing the tests for your parser I'd like to refer you to our contribution guide, where you can find instructions for creating a Folder Test.

When it comes to the parsers themselves, I should mentions that schema keys should only be made optional only when there's no other way to address the device's possible outputs. For the commands you're parsing here, if there is a column in the output table that may or may not be filled, then an optional key is helpful for that column only.

Lastly it would be best to remove the sdk/ folder as you mentioned.

else:
out = output

# This matches the report portion of the show vlan command.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the file could be cleaned up a bit by having the comments at the appropriate level of indentation for the code

Comment on lines +32 to +36
Optional('vlan_id'): str,
Optional('name'): str,
Optional('status'): str,
Optional('voice'): str,
Optional('jumbo'): str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do all of these need to be optional? Optional keys are best used when it's possible for only part of a line to show up. Overusing optional can lead to parsers passing when they should otherwise have failed

Comment on lines +1 to +5
Only one changelog file per pull request. Combine these two templates where applicable.

Templates
=========

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Only one changelog file per pull request. Combine these two templates where applicable.
Templates
=========

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.

2 participants