Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

What about a get_ipv6_neighbors_table() method? #311

Merged
merged 3 commits into from
Sep 30, 2017
Merged

What about a get_ipv6_neighbors_table() method? #311

merged 3 commits into from
Sep 30, 2017

Conversation

pierky
Copy link
Contributor

@pierky pierky commented Sep 20, 2017

No description provided.

@pierky pierky mentioned this pull request Sep 20, 2017
Open
@coveralls
Copy link

coveralls commented Sep 20, 2017 •
edited
Loading

Coverage Status

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

@pierky pierky mentioned this pull request Sep 25, 2017
Merged
@mirceaulinic
Copy link
Member

Hi @pierky - thanks for this PR.
We actually do have an implementation available for this this method: napalm-automation/napalm-eos#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
Copy link

coveralls commented Sep 27, 2017 •
edited
Loading

Coverage Status

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

@pierky
Copy link
Contributor Author

pierky commented Sep 27, 2017

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 mirceaulinic requested a review from a team September 27, 2017 16:54
@mirceaulinic mirceaulinic added this to the 0.26.0 milestone Sep 27, 2017
@mirceaulinic
Copy link
Member

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
Copy link
Contributor

ktbyers commented Sep 29, 2017

I assume age is in seconds?

Besides this it looks good to me.

@coveralls
Copy link

coveralls commented Sep 30, 2017 •
edited
Loading

Coverage Status

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

@pierky
Copy link
Contributor Author

pierky commented Sep 30, 2017

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

@dbarrosop
Copy link
Member

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

@ktbyers ktbyers merged commit a5972f4 into napalm-automation:develop Sep 30, 2017
@pierky pierky deleted the get_ipv6_neighbors_table branch October 2, 2017 10:54
@mirceaulinic mirceaulinic mentioned this pull request Oct 7, 2017
Open
@bewing bewing mentioned this pull request Jul 25, 2023
Closed
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
new_method
Projects
None yet
Milestone
0.26.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pierky @coveralls @mirceaulinic @ktbyers @dbarrosop