-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dhcpv6-server: T5992: Fix op-mode Kea DHCP lease output #4221
base: current
Are you sure you want to change the base?
Conversation
❌ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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']
Not used to amend commit message, does these renaming works ? |
CI integration 👍 passed! Details
|
Change Summary
Fixing for op commands:
show dhcpv6 server static-mappings
to includeipv6-address
from actual dhcpv6 static mappings in config.show dhcpv6 server leases
to includehostname
field (and removing trailing dot if any).Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
ipv6-address
field in config sectionservice dhcpv6-server [...] static-mapping
to report them in op commandshow dhcpv6 server static-mappings
hostname
field in leases for eithershow dhcp server leases
/show dhcpv6 server leases
op commandssort
arg inshow dhcpv6 server [...]
op commandsHow to test
Smoketest result
Checklist: