Skip to content
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

limits vlans on interface tables #17662

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

thordreier
Copy link

Fixes: #17655

Limit VLANs displayed on interface tables

@@ -56,9 +56,13 @@

INTERFACE_TAGGED_VLANS = """
{% if record.mode == 'tagged' %}
{% if value.count > 3 %}
<a href="{{ record.get_absolute_url }}#vlans">{{ value.count }}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the interface view, let's link to the VLAN list with a filter for the current interface. This will be consistent with the way we currently handle assigned IP addresses (see INTERFACE_IPADDRESSES).

Copy link
Author

Choose a reason for hiding this comment

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

That was also my initial plan, but VLANs doesn't seem to have interface filter like IP-address do.
So ipam/vlans/?(vm)interface_id=XXX needs to be implemented first for this to work.
That's why I implemented it with "vlan" anchor on interface page instead.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. I've just opened FR #17669 to add filters for this. I'll see if I can knock that out real quick today.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the pull request to support #17669

@sleepinggenius2
Copy link
Contributor

Just my two cents on this, as I know it's being implemented the same way as for IP addresses. For IP addresses, having just a number instead of a list of IPs is somewhat intuitive as to what is being conveyed, but in this context, I have a concern that having just a number may be misinterpreted as representing an actual VID and not a count that needs to be clicked on for additional details.

@thordreier
Copy link
Author

@sleepinggenius2 I've changed it to "5 VLANs" - not just "5"

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

To ensure that VLAN IDs are rendered correctly when exporting e.g. to CSV, we also need to add a method to BaseInterfaceTable (similar to value_ip_addresses). This should work:

    def value_tagged_vlans(self, value):
        return ",".join([str(obj.vid) for obj in value.all()])

@thordreier
Copy link
Author

I used str(obj) instead of str(obj.vid) to be consistent with previous behavior (before this change, you got "VLAN-name (VID)" when exporting to CSV, not just "VID")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit VLANs displayed on interface tables
3 participants