Fix #153 and optimise the implementation

napalm-automation / napalm-junos

Apache License 2.0

22 stars 42 forks source link

Fix #153 and optimise the implementation #155

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

As described in #154, for newer Junos, it is possible to get the instance name from the BGP neighbos, so we can enhance the implementation and reduce the run time by executing a single request fetching all BGP neighbors.

mirceaulinic commented 7 years ago

Yes, I will revisit this next week.

coveralls commented 7 years ago

Changes Unknown when pulling 2dc3b6475ce0c29a8dad15b59243e0c625f7a5b6 on mirceaulinic:bgp-153-fix into on napalm-automation:develop.

coveralls commented 7 years ago

Changes Unknown when pulling 2dc3b6475ce0c29a8dad15b59243e0c625f7a5b6 on mirceaulinic:bgp-153-fix into on napalm-automation:develop.

mirceaulinic commented 7 years ago

Alright, this one is fixed and ready to be reviewed @dbarrosop @ktbyers. Once this one gets merged, we can rerun the tests for the other pull requests pending in this release and they should pass.

coveralls commented 7 years ago

Changes Unknown when pulling 2dc3b6475ce0c29a8dad15b59243e0c625f7a5b6 on mirceaulinic:bgp-153-fix into on napalm-automation:develop.

coveralls commented 7 years ago

Changes Unknown when pulling 2dc3b6475ce0c29a8dad15b59243e0c625f7a5b6 on mirceaulinic:bgp-153-fix into on napalm-automation:develop.

ktbyers commented 7 years ago

@mirceaulinic One other comment...I think you should probably send the AS Numbers through the helper function in napalm_base helpers.

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

It looks like Juniper does support as-dot notation in some contexts (at least based on google search).

Also you might want to validate the version of IP address. This will help ensure that an IPv4 or IPv6 address is proper. Also see the same PR:

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

Those should both be in the released napalm-base now.

mirceaulinic commented 7 years ago

Pushed the changes as per @ktbyers's suggestion for the AS number: https://github.com/napalm-automation/napalm-junos/pull/155/commits/f68ac4dbb66b7749783ca75d43c743a86b7dcdf4 Regarding the IP version, on Junos there's no separation between IPv4 and IPv6 neighbors, they are all IP neighbors (which actually makes more sense). So we can't know in advance what version we should enforce for a neighbor.

mirceaulinic commented 7 years ago

CI broken, currently pending https://github.com/napalm-automation/napalm-base/pull/255

coveralls commented 7 years ago

Changes Unknown when pulling 9ca7ba5882820eefabdd7120d4b3c266f66403d8 on mirceaulinic:bgp-153-fix into on napalm-automation:develop.

mirceaulinic commented 7 years ago

napalm-base 0.24.1 released, going to merge this into develop, without pushing on PyPi.