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

Get interfaces ip #205

Merged
merged 3 commits into from
Sep 29, 2017
Merged

Get interfaces ip #205

merged 3 commits into from
Sep 29, 2017

Conversation

targuan
Copy link
Contributor

@targuan targuan commented Sep 27, 2017

Fix #204
As proposed in the issue by ktbyers I used show ip[v6] interface.

I changed the documentation of get_interfaces_ip in the code as N/A was not accepted on link-local ipv6 prefix_length because it was not an integer. I replaced N/A by 64 which seems to be the prefix length of such addresses.

@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Coverage increased (+1.9%) to 71.797% when pulling 8cdc7c2 on targuan:get_interfaces_ip into eaaaec6 on napalm-automation:develop.

@ktbyers ktbyers self-assigned this Sep 27, 2017
ipv4.update({ip: {"prefix_length": int(prefix)}})

# Remove interfaces without ip
interfaces = dict(filter(lambda x: x[1]['ipv4'] != {}, interfaces.items()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might this be more readable as

interfaces = {intf: details for intf, details in interfaces.items() if details['ipv4'] != {}}

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think I would probably change this slightly to something like:

            if(line[0] != ' '):
                ipv4 = {}
                interface_name = line.split()[0]
            m = re.match(INTERNET_ADDRESS, line)
            if m:
                ip, prefix = m.groups()
                ipv4.update({ip: {"prefix_length": int(prefix)}})
                interfaces[interface_name] = {'ipv4': ipv4}

i.e. only add it to 'interfaces' if there is a positive match

@ktbyers
Copy link
Contributor

ktbyers commented Sep 29, 2017

FYI, we agreed on /10 for link-local addresses after reviewing some documents on it:

napalm-automation/napalm-base#304

A bit of ambiguity on whether we should use /64 or /10 for this.

@ktbyers
Copy link
Contributor

ktbyers commented Sep 29, 2017

Only other comment I have is that I need to verify how we handle secondary addresses by default. In other words, whether we do anything to differentiate between a secondary or primary address in the returned data structure.

@targuan
Copy link
Contributor Author

targuan commented Sep 29, 2017

Thanks for your feebacks.
I've updated my PR according to your remarks.

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+1.9%) to 71.797% when pulling d06c7b9 on targuan:get_interfaces_ip into eaaaec6 on napalm-automation:develop.

@ktbyers
Copy link
Contributor

ktbyers commented Sep 29, 2017

The secondary address form is correct. So this should be right.

@ktbyers ktbyers merged commit 6e7c2e6 into napalm-automation:develop Sep 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_interfaces_ip executes a show command for each interface
4 participants