napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

Added a new method for ISIS adjacency #307

Open muffizone opened 7 years ago

muffizone commented 7 years ago

Pull request for get_isis_adjacencies method

napalm-automation/napalm-iosxr/pull/146

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.099% when pulling cf152c9dee870e81c010f9bd022b3b8084bf598a on muffadal-presswala:get_isis_neighbor into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.099% when pulling 9ad4a345ffa582bd2edabb858ba89c51447d8520 on muffadal-presswala:get_isis_neighbor into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 61.099% when pulling 39b5a04fd88521feed36e58108ec574ad69891a3 on muffadal-presswala:get_isis_neighbor into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

dbarrosop commented 7 years ago

LGTM, @napalm-automation/council any comments?

dbarrosop commented 7 years ago

@muffadal-presswala I think you mentioned already you have some implementation ready as well, feel free to push it linking the PR to this one for reference. We usually like seeing a working implementation before merging new getters.

dbarrosop commented 7 years ago

Disregard my last commend, I just saw you already did, linking for reference: napalm-automation/napalm-iosxr/pull/146

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 61.183% when pulling b9ae6731f73d24870e91a17564242afd44323760 on muffadal-presswala:get_isis_neighbor into e21f10f5f26b02d15ed0e73fe4e6476638d991e7 on napalm-automation:develop.

muffizone commented 7 years ago

Guys,

I was wondering if we need another method like get_isis_adjacencies_detail(). The plan was to add three additional keys in the neighbor attributes.

Same XML request is used to retrieve output for both of them.

Brief method output:

{system_id:
  {'interface_name': interface_name,
   'neighbor_state': neighbor_state,
   'circuit_type': circuit_type,
   'ietf_nsf_flag': ietf_nsf_flag,
   'network_type': network_type,
  }
}

Detailed method output

{system_id:
  {'interface_name': interface_name,
   'neighbor_state': neighbor_state,
   'circuit_type': circuit_type,
   'ietf_nsf_flag': ietf_nsf_flag,
   'network_type': network_type,
>    'neighbor_area': neighbor_area,
>    'neighbor_uptime': neighbor_uptime,
>    'topologies_supported': topologies_supported

  }
}
dbarrosop commented 7 years ago

Given the base information is the same I'd be inclined to only have the detailed version. Unless the detailed version has a huge cost in terms of querying the devices.

muffizone commented 7 years ago

Its the same query as base so should not make a difference

On Wed, Sep 20, 2017 at 9:23 AM, David Barroso notifications@github.com wrote:

Given the base information is the same I'd be inclined to only have the detailed version. Unless the detailed version has a huge cost in terms of querying the devices.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/napalm-automation/napalm-base/pull/307#issuecomment-330781356, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVHlQ4gYEhu-wyz-BtOH9Hmp1QQWxiuks5skMuUgaJpZM4PSCJ1 .

-- Regards, Muffadal S Presswala

dbarrosop commented 7 years ago

Then, unless someone has strong opinions about it I'd suggest sticking with the detailed one only.

muffizone commented 7 years ago

Ok will wait couple of days and push the changes soon for review

On Wed, Sep 20, 2017 at 11:30 AM, David Barroso notifications@github.com wrote:

Then, unless someone has strong opinions about it I'd suggest sticking with the detailed one only.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/napalm-automation/napalm-base/pull/307#issuecomment-330812297, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVHlcZuj5czAoZ0PdNI25xry9KGfTpuks5skOk6gaJpZM4PSCJ1 .

-- Regards, Muffadal S Presswala

muffizone commented 7 years ago

I have updated the method with additional output

mirceaulinic commented 7 years ago

Hi all - sorry for chiming in so late here. With the risk of being a PITA, wouldn't be better to return a dictionary with the same, but structured as per the openconfig-isis YANG model? This way we are sure we cover the fields defined there, we don't use a structure that we designed, and this could very well help with the napalm-yang too. Thoughts?

muffizone commented 7 years ago

Sure, sounds like a good idea.

Let me have a look and see what needs to change