Make use of mac() and ip() helpers. #46
Conversation
There was a problem hiding this comment.
Just noticed that you missed some details, please update @inetuid
| @@ -1277,7 +1277,7 @@ def _parse_per_peer_bgp_detail(peer_output): | |||
| unicode, item['last_event'])) | |||
| item['remote_address'] = ( | |||
| napalm_base.helpers.convert( | |||
| unicode, item['remote_address'])) | |||
| napalm_base.helpers.ip, item['remote_address'])) | |||
There was a problem hiding this comment.
should be napalm_base.helpers.ip(item['remote_address']))
There was a problem hiding this comment.
convert() looks to call the first argument (napalm_base.helpers.ip) with the second argument (item['remote_address']):
def convert(to, who, default=u''):
if who is None:
return default
try:
return to(who)
except:
return default
So I don't think it's appropriate to pass napalm_base.helpers.ip(item['remote_address'])).
You could argue that the whole convert part is unnecessary, but I went for minimal changes.
There was a problem hiding this comment.
Right, missed that napalm_base.helpers.ip is passed as argument for convert.
There was a problem hiding this comment.
And yeah. you can get rid of the convert - not needed anymore
| @@ -1292,10 +1292,10 @@ def _parse_per_peer_bgp_detail(peer_output): | |||
| unicode, item['routing_table'])) | |||
| item['router_id'] = ( | |||
| napalm_base.helpers.convert( | |||
| unicode, item['router_id'])) | |||
| napalm_base.helpers.ip, item['router_id'])) | |||
There was a problem hiding this comment.
should be napalm_base.helpers.ip(item['router_id']))
| item['local_address'] = ( | ||
| napalm_base.helpers.convert( | ||
| unicode, item['local_address'])) | ||
| napalm_base.helpers.ip, item['local_address'])) |
There was a problem hiding this comment.
should be napalm_base.helpers.ip(item['local_address']))
| @@ -789,8 +789,8 @@ def get_arp_table(self): | |||
| arp_table.append( | |||
| { | |||
| 'interface' : interface, | |||
| 'mac' : mac_format, | |||
| 'ip' : ip, | |||
| 'mac' : napalm_base.helpers.mac(mac_format), | |||
There was a problem hiding this comment.
Here you can also use mac_raw instead of mac_format and remove the unnecessary lines.
| @@ -957,7 +957,7 @@ def get_mac_address_table(self): | |||
| moves = mac_entry.get('moves', 0) | |||
| mac_table.append( | |||
| { | |||
| 'mac' : mac_format, | |||
| 'mac' : napalm_base.helpers.mac(mac_format), | |||
There was a problem hiding this comment.
Same here, can use mac_raw
|
Not sure if/what I need to do with this? |
|
@inetuid Can you please rebase? |
|
If someone rebases this I will merge it and release a new version : ) |
|
Assuming a rebase is fixing the merge (not updating the commit log) I made a change but it's now failing the unit tests. The tests pass if I run them locally on my development box but fail under Travis. Looks like the case difference in the mac() method is causing the problem. |
|
@inetuid You did good. The new testing framework compares the expected data with the information returned by the getters - and seems to be case sensitive (e.g.: For traceroute, as the unresponsive hops are marked with * (which obviously is not a valid IP address), you should do: hop_addr = hop_details[4+probe_index*5]
ip_address = napalm_base.helpers.convert(napalm_base.helpers.ip, hop_addr, hop_addr)Which returns the raw value (*) in case unable to convert to ip format. |
| @@ -1190,7 +1188,7 @@ def traceroute(self, destination, source='', ttl=0, timeout=0): | |||
| traceroute_result['success'][hop_index] = {'probes': {}} | |||
| for probe_index in range(probes): | |||
| host_name = hop_details[3+probe_index*5] | |||
| ip_address = hop_details[4+probe_index*5] | |||
| ip_address = napalm_base.helpers.ip(hop_details[4+probe_index*5]) | |||
There was a problem hiding this comment.
Replace with this @inetuid
hop_addr = hop_details[4+probe_index*5]
ip_address = napalm_base.helpers.convert(napalm_base.helpers.ip, hop_addr, hop_addr)
|
Changes made. |
Implements #31