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

Add IOS Parser "show vlan internal usage" #796

Merged
merged 20 commits into from
Dec 21, 2023

Conversation

cherifimehdi
Copy link
Contributor

@cherifimehdi cherifimehdi commented Nov 1, 2023

Description

New IOS/IOSXE Parser "show vlan internal usage"

Motivation and Context

I noticed that there is no parser for "show vlan internal usage" and this contribution will help network engineer

Impact (If any)

Screenshots:

Testing ShowVlanInternalUsage class:

test1

Testing ShowVlanInternalUsage_iosxe class:

test2

Testing the "show vlan internal usage" parser:

script

test_parser

Checklist:

  • I have updated the changelog.
  • I have updated the documentation (If applicable).
  • I have added tests to cover my changes (If applicable).
  • All new and existing tests passed.
  • All new code passed compilation.

@cherifimehdi cherifimehdi requested a review from a team as a code owner November 1, 2023 18:51
@cherifimehdi
Copy link
Contributor Author

Hello, two days ago I added a new parser for IOS "show vlan internal usage"

@cherifimehdi
Copy link
Contributor Author

Hi, please have you checked my submission for a parser? Thank you

Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. There are a few things that need to be address in this PR, but overall it's looking pretty good

src/genie/libs/parser/ios/show_vlan_internal_usage.py Outdated Show resolved Hide resolved
src/genie/libs/parser/ios/show_vlan_internal_usage.py Outdated Show resolved Hide resolved
# =================================================
# Parser for 'show vlan internal usage' command
# =================================================
class ShowVlanInternalUsage(ShowVlanInternalUsageSchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This parser and its schema should be under show_vlan.py, rather than creating a new file for it. Additionally, for ios devices which are typically identical to iosxe devices, we'll usually put the parser under iosxe and import it into ios. You can see an example of what I mean in src/genie/libs/parser/ios/show_vlan.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please should I remove the file show_vlan_internal_usage.py and pull directly my change under show_vlan.py?

src/genie/libs/parser/ios/show_vlan_internal_usage.py Outdated Show resolved Hide resolved
@cherifimehdi
Copy link
Contributor Author

Please shoud I continue with this PR or close it and open a new one to move the parser to show_vlan.py under IOSXE?

Here the contribution with "show vlan internal usage" parser as asked, I moved it to "show_vlan.py" and imported it from IOSXE to IOS
…ctory

As asked, I will delet this folder to create one in IOSXE Test
I moved this in "show_vlan.py" and imported it from IOSXE into ISO as asked
I added "show vlan internal usage" parser to "show_vlan.py" file as asked in IOSXE and I imported it into IOS
empty_output_output.txt
Add "golden_output1_expected.py" and "golden_output1_output.txt"
@cherifimehdi
Copy link
Contributor Author

Hello, I addressed all the changes asked for:

  1. I deleted the file "show_vlan_internal_usage.py" under IOS parser and I moved it in "show_vlan.py" under IOSXE
  2. I imported the parser "show_vlan_internal_usage" from IOSXE into IOS as asked under "show_vlan.py" under IOS
  3. I deleted the folder "ShowVlanInternalUsage" under tests of IOS and moved it into tests of IOSXE
  4. I did all the test as you can see it from the updated test with screenshots
    Thank you for your review

@cherifimehdi
Copy link
Contributor Author

cherifimehdi commented Nov 28, 2023 via email

@cherifimehdi
Copy link
Contributor Author

cherifimehdi commented Dec 5, 2023 via email

@tahigash
Copy link
Contributor

Hello, Thank you for reviewing and accepting my changes. Please see "Changes approved" by 02 reviewers,"All checks have passed" and "This branch has no conflicts with the base branch". Please, does this mean my parser has merged or this needs to be granted by the third reviewer? Thank you Le mar. 28 nov. 2023 à 17:32, Mehdi CHERIFI @.> a écrit :

Hello Please can you review my PR cause I addressed all the changes according to your review Thank you Le ven. 24 nov. 2023 à 16:08, Thomas Ryan @.
> a écrit : > @.**** requested changes on this pull request. > > Sorry for the delay. There are a few things that need to be address in > this PR, but overall it's looking pretty good > ------------------------------ > > In src/genie/libs/parser/ios/show_vlan_internal_usage.py > <#796 (comment)> > : > > > +import re > + > +# Metaparser > +from genie.metaparser import MetaParser > +from genie.metaparser.util.schemaengine import Any > + > + > +# ==================================================== > +# Schema for 'show vlan internal usage' command > +# ==================================================== > +class ShowVlanInternalUsageSchema(MetaParser): > + """Schema for: > + show vlan internal usage""" > + > + schema = { > + 'Internal Vlan': { > > Key names should all be lowercase with spaces replaced by underscores > ⬇️ Suggested change > > - 'Internal Vlan': { > + 'internal_vlan': { > > ------------------------------ > > In src/genie/libs/parser/ios/show_vlan_internal_usage.py > <#796 (comment)> > : > > > +# Metaparser > +from genie.metaparser import MetaParser > +from genie.metaparser.util.schemaengine import Any > + > + > +# ==================================================== > +# Schema for 'show vlan internal usage' command > +# ==================================================== > +class ShowVlanInternalUsageSchema(MetaParser): > + """Schema for: > + show vlan internal usage""" > + > + schema = { > + 'Internal Vlan': { > + Any(): { > + 'Usage': str, > > ⬇️ Suggested change > > - 'Usage': str, > + 'usage': str, > > ------------------------------ > > In src/genie/libs/parser/ios/show_vlan_internal_usage.py > <#796 (comment)> > : > > > +class ShowVlanInternalUsageSchema(MetaParser): > + """Schema for: > + show vlan internal usage""" > + > + schema = { > + 'Internal Vlan': { > + Any(): { > + 'Usage': str, > + }, > + } > + } > + > +# ================================================= > +# Parser for 'show vlan internal usage' command > +# ================================================= > +class ShowVlanInternalUsage(ShowVlanInternalUsageSchema): > > This parser and its schema should be under show_vlan.py, rather than > creating a new file for it. Additionally, for ios devices which are > typically identical to iosxe devices, we'll usually put the parser under > iosxe and import it into ios. You can see an example of what I mean in > src/genie/libs/parser/ios/show_vlan.py > ------------------------------ > > In src/genie/libs/parser/ios/show_vlan_internal_usage.py > <#796 (comment)> > : > > > + internal_vlan_dict=parsed_dict.setdefault('Internal Vlan', {}) > + Usage=m.groupdict()['Usage'] > + VLAN=m.groupdict()['VLAN'] > + internal_vlan_dict[VLAN] = {} > + internal_vlan_dict[VLAN]['Usage']=Usage > > Just a bit of cleaning up > ⬇️ Suggested change > > - internal_vlan_dict=parsed_dict.setdefault('Internal Vlan', {}) > - Usage=m.groupdict()['Usage'] > - VLAN=m.groupdict()['VLAN'] > - internal_vlan_dict[VLAN] = {} > - internal_vlan_dict[VLAN]['Usage']=Usage > + internal_vlan_dict = parsed_dict.setdefault('internal_vlan', {}) > + usage = m.groupdict()['Usage'] > + vlan = m.groupdict()['VLAN'] > + vlan_dict = internal_vlan_dict[vlan].set_default(vlan, {}) > + vlan_dict['usage'] = usage > > — > Reply to this email directly, view it on GitHub > <#796 (review)>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/ARSLWN67QSL6AWPDNKYPCIDYGCZ5NAVCNFSM6AAAAAA6ZXKTRGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBYGA4TGOJSGQ > . > You are receiving this because you authored the thread.Message ID: > @.***> >

The merge is supposed to be done by pyATS team after having 2 approvals.

@ThomasJRyan or @Lelor please merge if no issue for merging.

@cherifimehdi
Copy link
Contributor Author

Hi @ThomasJRyan @Lelor @sowmyadn010501 Does merging take time after changes approved by two reviewers and all test pass with no conflict with the base branch? Thank you

@ThomasJRyan ThomasJRyan merged commit 308388f into CiscoTestAutomation:master Dec 21, 2023
4 checks passed
@cherifimehdi
Copy link
Contributor Author

@ThomasJRyan Thank you for merging my contribution, and thanks to @Lelor @tahigash

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.

4 participants