dhcpv6-server: T5992: Fix op-mode Kea DHCP lease output #4221
Conversation
nvandamme
requested review from
dmbaturin,
sarthurdev,
zdc,
jestabro,
sever-sever and
nicolas-fort
|
❌ |
There was a problem hiding this comment.
Thanks for the PR.
While small fixes relating to the same area of code as the PR targets are usually no problem, each fix should be in separate commits with an identifying commit message.
The commits in this PR all have the same commit message, despite changing different things. Can you please reword the messages for what each commit changes?
| duid = entry.get('duid', 'N/A') | ||
| description = entry.get('description', 'N/A') | ||
| data_entries.append([pool, subnet, name, ip_addr, mac_addr, duid, description]) | ||
| elif family == 'inet6': |
There was a problem hiding this comment.
Instead of duplicating all these lines for the one ipv6-address key difference. Could this instead be handled by:
ip_addr = entry.get('ipv6-address' if family == 'inet6' else 'ip-address', 'N/A')
There was a problem hiding this comment.
Of course, normally i'll be doing so, but most part of handling code inside the same file has been split for the two inet families, given their formatting headers have not the same labels:
# IPv4 header for `show dhcp server leases`
headers = ['IP Address', 'MAC address', 'State', 'Lease start', 'Lease expiration', 'Remaining', 'Pool',
'Hostname', 'Origin']
# IPv6 header for `show dhcpv6 server leases`
headers = ['IPv6 address', 'State', 'Last communication', 'Lease expiration', 'Remaining', 'Type', 'Pool',
'DUID']
There was a problem hiding this comment.
Could this instead be handled by: ip_addr = entry.get('ipv6-address' if family == 'inet6' else 'ip-address', 'N/A')
We need room for future improvements, too ;)
Not used to amend commit message, does these renaming works ? |
|
CI integration 👍 passed! Details
|
|
Commits will be squashed and merged |
c-po
dismissed
sarthurdev’s stale review
Will be addressed in separate commit potentially combining all address family sections
Change Summary
Fixing for op commands:
show dhcpv6 server static-mappingsto includeipv6-addressfrom actual dhcpv6 static mappings in config.show dhcpv6 server leasesto includehostnamefield (and removing trailing dot if any).Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
ipv6-addressfield in config sectionservice dhcpv6-server [...] static-mappingto report them in op commandshow dhcpv6 server static-mappingshostnamefield in leases for eithershow dhcp server leases/show dhcpv6 server leasesop commandssortarg inshow dhcpv6 server [...]op commandsHow to test
Smoketest result
Checklist: