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

Adding new template for Alcatel aos: show system Co-authored-by: Michael Bear <38406045+mjbear@users.noreply.github.com> #1900

Merged
merged 8 commits into from
Nov 25, 2024

Conversation

evilmonkey19
Copy link
Contributor

No description provided.

@mjbear
Copy link
Contributor

mjbear commented Nov 18, 2024

@evilmonkey19
This PR also has multiple template changes.

@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_system.textfsm, .*, alcatel_aos, show system
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the AOS command line support any shorthand for show system?

Value CONTACT (.+,(?: https://\S+)?)
Value NAME ([^,]+)
Value LOCATION ([^,]+)
Value Service (\d+)
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.

Suggested change
Value Service (\d+)
Value SERVICES (\d+)

Make plural to match raw output label.
Capitalize capture group name.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this suggestion (and others) help, click the button here to accept them. Thank you!

Comment on lines 1 to 6
Value DESCRIPTION (.+,.+,[^,]+)
Value OBJECT_ID ([^,]+)
Value UP_TIME (\d+ days \d+ hours \d+ minutes and \d+ seconds)
Value CONTACT (.+,(?: https://\S+)?)
Value NAME ([^,]+)
Value LOCATION ([^,]+)
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.

Suggested change
Value DESCRIPTION (.+,.+,[^,]+)
Value OBJECT_ID ([^,]+)
Value UP_TIME (\d+ days \d+ hours \d+ minutes and \d+ seconds)
Value CONTACT (.+,(?: https://\S+)?)
Value NAME ([^,]+)
Value LOCATION ([^,]+)
Value DESCRIPTION (.+)
Value OBJECT_ID (\S+)
Value UP_TIME (.+)
Value CONTACT (.+)
Value NAME (\S+)
Value LOCATION (.+)

Simplify regexes.
Some capture groups do not need to accept spaces (thus \S+), but others do (hence .+).
Simpler and flexible should mean less breakage over time.

Value Service (\d+)
Value DATE_TIME (\S+\s*\S+\s*\d+\s*\d+\s*\d+:\d+:\d+\s*(\(\w+\)))
Value AVAILABLE (\d+)
Value COMMENTS ([^,]+)
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 COMMENTS ([^,]+)
Value COMMENTS (.+)

Simplify regex.

Value NAME ([^,]+)
Value LOCATION ([^,]+)
Value Service (\d+)
Value DATE_TIME (\S+\s*\S+\s*\d+\s*\d+\s*\d+:\d+:\d+\s*(\(\w+\)))
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.

Suggested change
Value DATE_TIME (\S+\s*\S+\s*\d+\s*\d+\s*\d+:\d+:\d+\s*(\(\w+\)))
Value DATE_TIME ((\S+\s*)+)

Simplify regex.

Fortunately you can (and are) anchoring the match line with that trailing comma within the rule line for this one. 😀

@@ -0,0 +1,27 @@
Value DESCRIPTION (.+,.+,[^,]+)
Value OBJECT_ID ([^,]+)
Value UP_TIME (\d+ days \d+ hours \d+ minutes and \d+ seconds)
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 UP_TIME (\d+ days \d+ hours \d+ minutes and \d+ seconds)
Value UPTIME (.+)

Move UP_TIME to UPTIME as that is a common capture group name. (We're hoping to standardize/normalize where we can.)

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.

^\s*System:$$
^\s*Description:\s*${DESCRIPTION},$$
^\s*Object\s*ID:\s*${OBJECT_ID},$$
^\s*Up\s*Time:\s*${UP_TIME},$$
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*Up\s*Time:\s*${UP_TIME},$$
^\s*Up\s*Time:\s*${UPTIME},$$

Match capture group change (as noted above in the file) to the common occurrence.

^\s*Contact:\s*${CONTACT},$$
^\s*Name:\s*${NAME},$$
^\s*Location:\s*${LOCATION},$$
^\s*Services:\s*${Service},$$
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*Services:\s*${Service},$$
^\s*Services:\s*${SERVICES},$$

Change capture group name and make all uppercase.

@evilmonkey19
Copy link
Contributor Author

I added all your suggestions 😀 Thanks again! I added also the short-handed version

@mjbear
Copy link
Contributor

mjbear commented Nov 20, 2024

I added all your suggestions 😀 Thanks again! I added also the short-handed version

@evilmonkey19
Nice!

Was the Commit suggestion button available?
Based on the commit history it doesn't seem like it.
For local development, when you accept changes from GH, you'd run a git pull to pull down changes to be up-to-date on your local system.

Commit suggestion button

@evilmonkey19
Copy link
Contributor Author

Yees it was available hehe. Simply just that sometimes i like to do things manually as everthing is automated :P

@mjbear
Copy link
Contributor

mjbear commented Nov 20, 2024

Yees it was available hehe. Simply just that sometimes i like to do things manually as everthing is automated :P

Darn.
We co-authored on this one and now commit history doesn't show it. 😞

@evilmonkey19
Copy link
Contributor Author

evilmonkey19 commented Nov 22, 2024

Is there anything i could do to add you?
Simply add a space or something similar and i add it :)

@evilmonkey19
Copy link
Contributor Author

I've seen that for making the tests better we can rename the _1 to _R6 and the _2 to _R8 ;)

@mjbear
Copy link
Contributor

mjbear commented Nov 22, 2024

Is there anything i could do to add you? Simply add a space or something similar and i add it :)

I'll see if there's anything else, but for now things appear to be fairly complete on this PR.
Though I did come up with a documentation idea.

If you were to add co-author text to the PR description (it might get picked up).
For example, GH auto-added similar text at this previous commit for another PR.

Co-authored-by: Michael Bear <38406045+mjbear@users.noreply.github.com>

@evilmonkey19 evilmonkey19 changed the title Adding new template for Alcatel aos: show system Adding new template for Alcatel aos: show system Co-authored-by: Michael Bear <38406045+mjbear@users.noreply.github.com> Nov 25, 2024
@jmcgill298 jmcgill298 merged commit 6697a54 into networktocode:master Nov 25, 2024
14 checks passed
@evilmonkey19 evilmonkey19 deleted the alcatel_aos_show_system branch November 26, 2024 15:07
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.

3 participants