search

napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

What about a get_ipv6_neighbors_table() method? #311

Closed pierky closed 7 years ago

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.099% when pulling bcebcd1e92fa272cdab505526acfa2885f5e623d on pierky:get_ipv6_neighbors_table into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

mirceaulinic commented 7 years ago

Hi @pierky - thanks for this PR. We actually do have an implementation available for this this method: https://github.com/napalm-automation/napalm-eos/pull/164, but we didn't have the test definition and the header. Your proposal looks good to me, as it is in-line with the get_arp_table brother. Please fix the conflict and then I think we are good to go. @napalm-automation/council any opinions?

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.141% when pulling 62968a7d7051100bbc12607190f41330cf2b8628 on pierky:get_ipv6_neighbors_table into 9102ca1a6ea709e4ad55da98a0ea6da77697e17e on napalm-automation:develop.

pierky commented 7 years ago

Hi @mirceaulinic, I didn't notice the EOS implementation, sorry.

The output format I proposed here is different from the one used in napalm-automation/napalm-eos#164: how do we proceed now?

mirceaulinic commented 7 years ago

You are correct @pierky , the EOS implementation is not the same. But I like yours as it is very similar to the get_arp_table structure, so I would suggest to move forward with yours.

Note that the structure however it is not complete (due to a design fault we made in get_arp_table), but I vote for this to be accepted so we have the IPv6 equivalent for the get_arp_able. The complete version will probably be covered using the yang models.

ktbyers commented 7 years ago

I assume age is in seconds?

Besides this it looks good to me.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.141% when pulling b47f89b84280b8cbf337559c59d7756fb3795c6d on pierky:get_ipv6_neighbors_table into 9102ca1a6ea709e4ad55da98a0ea6da77697e17e on napalm-automation:develop.

pierky commented 7 years ago

@ktbyers I explicitly stated it within the docstring now. And also fixed the IOS implementation ;-) Thanks for pointing that out.

dbarrosop commented 7 years ago

LGTM, I also prefer this one as it's closer to the ipv4 equivalent.