search

napalm-automation / napalm-nxos

Apache License 2.0
9 stars 21 forks source link

Handle 4 bytes ASN format #87

Closed targuan closed 7 years ago

targuan commented 7 years ago
coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.4%) to 67.377% when pulling 6f2fd87f69915f412f78b21cd870779c7d33ee0d on targuan:feature/4_bytes_AS into 529d613d28ae8c237a3936f51d749d5661fd9b6a on napalm-automation:develop.

ktbyers commented 7 years ago

I think we should implement this as a helper in napalm_base. I say this as other platforms will need this.

There is a PR in napalm-ios to reimplement the get_bgp_neighbors that will also probably need this conversion.

https://github.com/napalm-automation/napalm-ios/pull/137

ktbyers commented 7 years ago

@targuan Thanks for submitting this.

I created a function in napalm-base that uses your code and makes it more reusable across the different napalm-drivers.

It is now in the released version of napalm-base. See here:

https://github.com/napalm-automation/napalm-base/pull/246/files

Can you update this to call that function?

Either that or we can close this PR and resubmit a new one.

We also need to update the requirements.txt file to use napalm-base >= 0.24.0

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 67.216% when pulling 227777b874debe0ba06f00716f3f27cdded70f87 on targuan:feature/4_bytes_AS into 529d613d28ae8c237a3936f51d749d5661fd9b6a on napalm-automation:develop.

targuan commented 7 years ago

@ktbyers thanks for your feed back. I've updated this PR accordingly.

ktbyers commented 7 years ago

@targuan This looks fine...

do you know why we aren't parsing uptime on NX-OS. I see that we have is_up set to True, but uptime set at -1 (i.e. no value)?

targuan commented 7 years ago

Not sure why some values are hardcoded, maybe it's related to the cryptic format returned by the equipement for some field. I'm actually planning to propose a more complete getter for the bgp neighbors, base on another command, as the current one doesn't return all the address families I need.

ktbyers commented 7 years ago

Okay, let me merge this (as those other issues are independent of this).