Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Alcatel aos show vlan port members #1899

Conversation

evilmonkey19
Copy link
Contributor

Adding new template

@mjbear
Copy link
Contributor

mjbear commented Nov 18, 2024

@evilmonkey19
There are two templates in this PR. Normally this project expects only one template in a PR.
Was that intended?

Edit: Also, I have some suggestions once it is determined if this PR is split into two. Thank you.

@evilmonkey19
Copy link
Contributor Author

Fixed ✅ It was not intended but rather that i'm doing local development.

@@ -11,6 +11,7 @@
#
Template, Hostname, Platform, Command

alcatel_aos_show_vlan_port.textfsm, .*, alcatel_aos, show vlan (port|members)
Copy link
Contributor

@mjbear mjbear Nov 20, 2024

Choose a reason for hiding this comment

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

@evilmonkey19
Is it possible to have shorthand commands on the AOS command line?
ex: sh vl po

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically you can do sh vl po if you do tab after each command. However, I will add the possibility :)

Comment on lines 3 to 4
Value TYPE (default|qtagged)
Value STATUS (forwarding|inactive)
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
Value TYPE (default|qtagged)
Value STATUS (forwarding|inactive)
Value TYPE (\S+)
Value STATUS (\S+)

Probably better to make the pattern more flexible for the long term.
In my testing using this pattern causes no ill effects with the test data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added suggestion! :)

Comment on lines 8 to 9
^\s*--------+------------+------------+--------------*$$
^\s*------+-------+---------+-------------*$$
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
^\s*--------+------------+------------+--------------*$$
^\s*------+-------+---------+-------------*$$
^\s*[-+]+\s*$$

Condense two separator lines into one.
Added accommodation for trailing white space with \s* just in case there's ever a trailing space or two.

If you support my suggestion, click the button here to accept my suggestion. Thank you. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love it! Way cleaner and better :)

Copy link
Contributor

Choose a reason for hiding this comment

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

General feedback here.

We/you could probably condense this 102 line file into about ten lines by picking lines with unique patterns.
Several duplicate lines with the same patterns do not add value.

(This is some wisdom I've learned while wading through long raw and structured files plus waiting for the test cases to run. 🤣)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion! Added on the new commit :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar feedback to the previous raw output file.
Lots of the same duplicate lines don't add anything valuable to test against.

Example:
We'd be just fine with say one of these.

    1    1/27   default     inactive
    1    1/28   default     inactive

Apply that same thought to some of the other lines.
Please think about it. We can certainly continue the discussion. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted! ✏

@@ -0,0 +1,12 @@
Value VLAN_ID (\d+)
Value PORT ((\d+\/)?\d+\/\d+)
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
Value PORT ((\d+\/)?\d+\/\d+)
Value PORT ((\d+\/?)+)

This supports 1/1 and 1/1/1 ... plus more.

We can shorten this pattern quite a bit and even have the potential support 1/1/1/1 and beyond.
(Not that an interface would be named that anytime soon.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added this new suggestion :)

I doubt in the near future will see 1/1/1/1/1/1 but who knows xd

@evilmonkey19
Copy link
Contributor Author

Thank you for the suggestions! Later whenever I have sometime during the evening-night i will read them carefully :)

@jmcgill298 jmcgill298 merged commit 9140fef into networktocode:master Nov 20, 2024
14 checks passed
@evilmonkey19 evilmonkey19 deleted the alcatel_aos_show_vlan_port_members branch November 20, 2024 16:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants