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

Fixes: #18447 - Fix sorting by mac_address field #18450

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

DanSheps
Copy link
Member

Fixes: #18447 - Fix sorting by mac_address field

  • Disable sorting by mac_address for legacy mac_address field for Device and VM Interfaces
  • Ensure primary_mac_address field is included in field list for Device and VM Interfaces

* Disable sorting by `mac_address` for legacy `mac_address` field for Device and VM Interfaces
* Ensure `primary_mac_address` field is included in field list for Device and VM Interfaces
@DanSheps DanSheps requested a review from bctiemann January 21, 2025 16:35
@bctiemann
Copy link
Contributor

@DanSheps Actually I noticed that InterfaceTable already has primary_mac_address as a fully functional and sortable field (with properly capitalized label); I think we just need to add that same field to VMInterfaceTable, and then DeviceInterfacesTable and VirtualMachineVMInterfaceTable need similar fields with appropriate accessors so sorting works.

I'm not sure if your solution catches all four of these cases, or if the way it is on InterfaceTable is the most correct, but I feel like we can preserve the functionality across the four.

@DanSheps
Copy link
Member Author

DanSheps commented Jan 21, 2025

@DanSheps Actually I noticed that InterfaceTable already has primary_mac_address as a fully functional and sortable field (with properly capitalized label); I think we just need to add that same field to VMInterfaceTable, and then DeviceInterfacesTable and VirtualMachineVMInterfaceTable need similar fields with appropriate accessors so sorting works.

That is what I did (more or less). I didn't want to remove mac_address from those as they are not deprecated (yet) AFAIK, so preserving them and disabling sorting seems to be the better way to go.

If we add the accessor, it unfortunately overrides the primary_mac_address field (at least from what I can tell) with the verbose name set for mac_address.

I'm not sure if your solution catches all four of these cases, or if the way it is on InterfaceTable is the most correct, but I feel like we can preserve the functionality across the four.

IMO mac_address should be removed in the future and we stick with primary_mac_address. It is redundant information, we should just properly deprecate the field so people aren't surprised in a patch release.

@bctiemann
Copy link
Contributor

OK -- so should we disable sorting on that field in InterfaceTable too then? Right now that one is sortable and the other three are not.

@DanSheps
Copy link
Member Author

DanSheps commented Jan 21, 2025

Good catch, yes I would think so.

Wait, InterfaceTable doesn't have mac_address so no, we don't need to. MAC Address there is the primary_mac_address field.

However maybe we just remove mac_address all together and use primary_mac_address but would that mess up anybody's instance if they were set with that as a column/sort I wonder.

@DanSheps DanSheps self-assigned this Jan 21, 2025
@bctiemann
Copy link
Contributor

Invalid field names that are persisted in user prefs are filtered out in django-tables before being applied, I found in working on an earlier issue. I think it's safe to change field names (though it's reasonably easy to test too, just sort on that column and then rename it, see if anything breaks).

@DanSheps DanSheps linked an issue Jan 23, 2025 that may be closed by this pull request
@bctiemann bctiemann merged commit a816889 into main Jan 24, 2025
6 checks passed
@bctiemann bctiemann deleted the 18447-fix-legacy-macaddress-sorting branch January 24, 2025 13:19
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.

Duplicate MAC address attribute Sort by MAC address crashes "interfaces" tab view...
2 participants