Skip to content

Conversation

@renatoalmeidaoliveira
Copy link
Contributor

Fixes: #18656 Unable to import IP Address and assign to FHRP Group

  • Created the fhrpgroup field in IPAddressImportForm

Observations:

Since FHRPGroup Model doesn't implement uniqueness method, choosing the appropriated accessor for the fhrpgroup its a bit tricky.
I choose name instead of the group_id beacuse, IMO, group_id is more likely to get repeated, but maybe a more accurate approach could be open a new Issue to setup some uniqueness criteria for FHRPGroup, make the import form following that pattern.

@renatoalmeidaoliveira renatoalmeidaoliveira requested review from a team and arthanson and removed request for a team March 19, 2025 02:21
@jnovinger jnovinger requested review from jnovinger and removed request for arthanson March 21, 2025 15:27
Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

One minor change requested, plus please add this new import field to cls.csv_data for the tests in ipam.tests.test_view.py::IPAddressTestCase.setUpTestData.

label=_('FHRP Group'),
queryset=FHRPGroup.objects.all(),
required=False,
to_field_name='name',
Copy link
Member

Choose a reason for hiding this comment

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

Agreed with your choice to use name. This will require users to populated the name field for any FHRP Groups they want to assign, but that seems pretty reasonable.

I did some grep-ing through the code and it seems to_field_name='name' is almost universally the default, with only a couple of specific expections. If we need to add the ability to assign at import via FHRPGroup.group_id, that can definitely be a separate FR.

@renatoalmeidaoliveira
Copy link
Contributor Author

@jnovinger about the testing cases, I updated the cls.csv_data with fhrp_group but wasn't able to update csv_update_data, because test_bulk_update_objects_with_permission lookup for model fields based in the header names, since IPAddress doesn't have a field fhrp_group that test fails code

Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

Looks great, @renatoalmeidaoliveira !

@jnovinger jnovinger merged commit 447e108 into netbox-community:main Mar 21, 2025
3 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to import IP Address and assign to FHRP Group

2 participants