networktocode / ntc-rosetta

The missing bridge between industry standard CLIs and YANG
https://ntc-rosetta.readthedocs.io/en/latest/index.html
Apache License 2.0
103 stars 23 forks source link

OpenConfig-Interfaces additions #7

Closed FragmentedPacket closed 4 years ago

FragmentedPacket commented 5 years ago

This PR added the following leafs to the parser/translator for openconfig-interfaces config:

dbarrosop commented 5 years ago

This looks great, awesome work. I only have one comment, not sure how the "loopback-mode" works, would you mind adding to one of the test cases a loopback interface? That will help clarifying how it works and making sure it does work :)

FragmentedPacket commented 5 years ago

@dbarrosop My understanding of the loopback-mode is just a boolean value. If the interface is configured for a "loopback mode", it would be such as, loopback mac and the value will be True. If that is not detected within the configuration, the value equates to False.

I think I've accounted for it not being present, but I can add it to one of the FastEthernet interfaces just to test that it evaluates to true. I will get that taken care of and rebase my branch.

dbarrosop commented 5 years ago

I meant how it works in your code :P In ios it should be set if the interface name is Loopback but I am not sure that's what you are doing. In any case, adding a Loopback interface and seeing if it's set accordingly will clarify the issue.

FragmentedPacket commented 5 years ago

Sorry I'm a little confused on it. The description within openconfig is as follows:

    leaf loopback-mode {
      type boolean;
      default false;
      description
        "When set to true, the interface is logically looped back,
        such that packets that are forwarded via the interface
        are received on the same interface.";

My understanding of that is it isn't actually the name of the interface (Loopack X), but a sub configuration of a physical interface.

FragmentedPacket commented 5 years ago

I've added tests for both conditions (True/False)

dbarrosop commented 5 years ago

My understanding of that is it isn't actually the name of the interface (Loopack X), but a sub configuration of a physical interface.

Not necessarily. Basically any interface with the behavior will qualify. Note that there is no mention on the type of interface or any property of it, just "the interface is logically looped back". In any case, the logic of that code should be able to understand when an interface is inherently looped back (like the Loopback interface) or when it's set via configuration. Right now I think we are good just accounting for the Loopback interface, we can extend the method later on if we need to account for something else as well.

jathanism commented 5 years ago

@dbarrosop Based on your comments from 26 June, it appears this needs changes before merging. If you agree, can you please set your review to "changes requested"? Thank you.