CiscoTestAutomation / genieparser

sub-component of Genie that parse the device output into structured datastructure
Apache License 2.0
249 stars 387 forks source link

[Comware] The current parser only works on Comware 5 due to changes in the format. #827

Closed JanisFalkenhagenWork closed 7 months ago

JanisFalkenhagenWork commented 7 months ago

Hey there,

we recently found pyATS and wanted to use it in our environment too. As expected, our Cisco devices work out of the box. But HPE Comware 7 switches are acting up. We found the reason (changes in the output formatting between major versions) in the existing code and decided that we wanted to fix it and later on contribute it to this repo. For this reason, we wanted to ask for your opinion on a good way to fix/update/change this.

We think there are basically three ways we could implement this:

  1. Rework the regexes uses atm to be able to recognize both versions in parallel or 2. Update the regexes to recognize only the newest version. or 3. Split up the module by version.

In our opinion, both approaches have benefits as well as drawbacks. Reworking the regexes to handle both versions would allow backwards compatibility and ensures old code doesnt get broken by the changes. However, it may lead to confusion because the current code is very much not extensive, thus the new functions would all not work on the old versions. Also it would make the code less readable and maintainable. Contrary, updating the code to the newest version only, means that legacy code wont work anymore while ensureing better maintainability. The third option is a good blend of both worlds, in our opinion. However, we didnt spot any other places where this has been done. So we think that this is not the style, you are going for. And, frankly, atm we are too new to pyATS and genieparsers and thus clueless on how we would implement another version.

I hopefully somewhat summarized our problem. :D We hope to hear from you on what you think the best and most fitting approach for this repo is. And if you have one or two pointers on where to start looking for inspiration, or other places that had the same problem, that would be very much appreciated.

Have a beautiful day Janis

SohanTirpude commented 7 months ago

Hello @JanisFalkenhagenWork,

We usually prefers first option which is to have support for old as well as new version so it will allow backward compatibility as you already highlighted out. Regarding readability of the code which I don't think won't be affected as long as we are submitting code of good quality. But yes, we need to focus on the backward compatibility when writing any new or modifying any parsers/API.

Kindly let me know if you have any questions.

Thank you.

renatoalmeidaoliveira commented 7 months ago

About backward compability, maybe adding another platform comware7 and keeping comware as "comware5", because there're differences to other commands too, that way who uses comware5 keeps calling the platform as comware. The only thing to keep in mind if you choose to use comware7 is setting the unicon connector to use comware as well, since there aren't any changes between then.

Harishv01 commented 7 months ago

Thank you for your suggestion regarding the platform naming for HPE Comware switches. We appreciate your input on maintaining backward compatibility by adding "comware7" and keeping "comware" as "comware5.

We will carefully consider this solution, evaluating its impact on backward compatibility and code maintainability. Your idea aligns well with our aim to provide users with flexibility in specifying the platform version they are working with.

Kindly let me know if you have any questions.

Thank you.

JanisFalkenhagenWork commented 7 months ago

Hey,

thank you all for your input. Please let us know what your evaluation lead to @Harishv01, then we'll get to it following your preferred structure.

But just to be clear: You are still looking for community contributions right? Your last message sounds like you are going to write the parsers.

Have a nice day Janis

Harishv01 commented 7 months ago

Sorry for misunderstanding. Yes, we are looking for contributions from the community. Kindly let me know if you have any questions/doubts on this and on your ticket.

Thank you

Harishv01 commented 7 months ago

If you don't have any questions, I will proceed with the closure of this ticket.

Thank you.

JanisFalkenhagenWork commented 7 months ago

Hey,

you said in one of your previous messages: "We will carefully consider this solution, evaluating its impact on backward compatibility and code maintainability. Your idea aligns well with our aim to provide users with flexibility in specifying the platform version they are working with."

Did you do that already and if so, to which conclusion did you come? So which structure should we follow? Or is this something, you'd like to discuss once we issue the pull request?

But otherwise, you can gladly close the issue :D Thanks Janis

Harishv01 commented 7 months ago

In the same codebase and within the same parser, you can incorporate your modifications. This will apply to both existing and new versions. After making your changes, submit a pull request (PR). If any issues arise in your PR, the reviewers will check and provide feedback.

Thank you.

JanisFalkenhagenWork commented 7 months ago

OK. Thank you.

No questions left.

Harishv01 commented 7 months ago

You mentioned "No questions left " hence, I am closing this ticket.

Thank you