-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes: #19129 - Richer display of MAC addresses in InterfaceTable when multiple MACs are present #21270
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
base: main
Are you sure you want to change the base?
Conversation
netbox/dcim/tables/devices.py
Outdated
| ) | ||
| primary_mac_address = tables.Column( | ||
| verbose_name=_('MAC Address'), | ||
| accessor=Accessor('mac_address_display'), |
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.
This adds significant overhead as currently implemented, triggering a count of MAC addresses for each interface in the table. Why don't we just introduce a non-default column that lists all assigned MAC addresses (perhaps annotating the primary), like we do for IP addresses? That seems much cleaner IMO, and it ensures the MAC addresses are efficiently prefetched.
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.
Agreed, that's a better pattern. Done, also synchronized the vminterface.html MAC Address field HTML with interface.html which was overlooked before.
netbox/templates/dcim/interface.html
Outdated
| {% if object.primary_mac_address %} | ||
| <span class="font-monospace">{{ object.primary_mac_address|linkify }}</span> | ||
| <span class="badge text-bg-primary">{% trans "Primary" %}</span> | ||
| {% if object.mac_address_display %} |
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.
Same general note as above. Rather than only showing the primary MAC and hinting whether there are other MACs assigned, let's just display all the MACs and annotate the primary.
… tables in list view and detail view
| verbose_name=_('IP Addresses') | ||
| ) | ||
| primary_mac_address = tables.Column( | ||
| verbose_name=_('MAC Address'), |
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.
We should annotate this as primary somehow to avoid confusion with the multi-MAC column.
| verbose_name=_('MAC Address'), | |
| verbose_name=_('MAC Address (Primary)'), |
| verbose_name=_('MAC Address'), | ||
| linkify=True | ||
| ) | ||
| mac_addresses = tables.TemplateColumn( |
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.
IMO we should just make this a ManyToManyColumn.
|
|
||
| INTERFACE_MACADDRESSES = """ | ||
| {% if value.count > 3 %} | ||
| <a href="{% url 'ipam:macaddress_list' %}?{{ record|meta:"model_name" }}_id={{ record.pk }}">{{ value.count }}</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.
If we keep the TemplateColumn
| <a href="{% url 'ipam:macaddress_list' %}?{{ record|meta:"model_name" }}_id={{ record.pk }}">{{ value.count }}</a> | |
| <a href="{% url 'dcim:macaddress_list' %}?{{ record|meta:"model_name" }}_id={{ record.pk }}">{{ value.count }}</a> |
Fixes: #19129
On the list view for Interface and VMInterface, the presence of multiple MAC addresses on an interface is now communicated in various ways depending on whether one is assigned:
Note: this same logic is reflected on the interface detail page.