Skip to content

Closes #8423: Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine #19005

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Apr 11, 2025
Merged

Closes #8423: Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine #19005

merged 17 commits into from
Apr 11, 2025

Conversation

jnovinger
Copy link
Member

@jnovinger jnovinger commented Mar 25, 2025 •
edited
Loading

Closes: #8423

  • Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine
  • Changes relation type for all three to a single GenericForeignKey that is required
    • Has the same semantics as before, that were enforced in Service.clean(), now enforced by the GFK and in model forms
  • Patches up REST API and GraphQL representations
  • Fixes up ServiceImportForm to work with the new Service.parent GFK

@jnovinger jnovinger marked this pull request as draft March 25, 2025 15:17
@jnovinger jnovinger changed the title WIP: #8423 Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine RfC: #8423 Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine Mar 25, 2025
@jeremystretch jeremystretch requested review from jeremystretch and removed request for jeremystretch March 26, 2025 21:14
@jnovinger jnovinger force-pushed the 8423-assign-service-to-fhrp-group branch from 26e36df to 87cb4b7 Compare April 4, 2025 14:58
@jnovinger jnovinger changed the title RfC: #8423 Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine #8423 Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine Apr 7, 2025
@jnovinger jnovinger marked this pull request as ready for review April 7, 2025 13:27
@jnovinger jnovinger requested review from a team and arthanson and removed request for a team April 7, 2025 13:51
@arthanson
Copy link
Collaborator

FHRP Group details view should show the related services, either as related objects or a sub-table.

arthanson
arthanson requested changes Apr 7, 2025
View reviewed changes
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Also - not sure if we want to change from tabbed interface for selection of assignment to dropdown - see discussion in maintainers channel chat.

@jnovinger jnovinger changed the title #8423 Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine Closes #8423: Allow assigning Service to FHRP Group, in addition to Device and VirtualMachine Apr 7, 2025
@jnovinger jnovinger requested a review from arthanson April 8, 2025 00:08
@jnovinger
Copy link
Member Author

FHRP Group details view should show the related services, either as related objects or a sub-table.

Fixed

@arthanson
Copy link
Collaborator

@jnovinger test failing, not sure why: netbox/ipam/views.py:1435:9: SyntaxError: Cannot use match statement on Python 3.9 (syntax was added in Python 3.10)

@jeremystretch
Copy link
Member

@arthanson see #19112

@jnovinger
Copy link
Member Author

@jnovinger test failing, not sure why: netbox/ipam/views.py:1435:9: SyntaxError: Cannot use match statement on Python 3.9 (syntax was added in Python 3.10)

Ah, this is actually a linting thing. I guess this is what the PR from Jeremy re: ruff and 3.10 python target is all about.

@jnovinger
Copy link
Member Author

Just realized I also missed updating the CSV import form for Services. Working on that now.

@jnovinger
Copy link
Member Author

@arthanson , this is ready for re-review now.

arthanson
arthanson requested changes Apr 9, 2025
View reviewed changes
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Just a couple minor issues - also needs docs in docs/models/ipam/service.md add in the parent field, can look at scope on vlan group for template.

netbox/ipam/forms/bulk_import.py Outdated
self.cleaned_data['parent'] = parent
else:
# If a parent object type is passed and we've made it to here, then raise a validation
# error since an associated parent object or parent object id has nto be passed
Copy link
Collaborator

Choose a reason for hiding this comment

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

type "nto be passed" -> "not been passed"

netbox/ipam/forms/model_forms.py
@@ -21,7 +21,7 @@
from utilities.forms.utils import get_field_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you set parent type without the associated device, VM, FHRP Group and try to save there is no error given, should be a "Cannot set parent type without an associated object" message

Copy link
Member Author

Choose a reason for hiding this comment

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

Phew, this only took forever to figure out. After a fair amount of debugging compiled .ts, reading bootstrap docs, and finally understanding how those in-page "This field is required." messages get displayed, I discovered this is because of #18916.

The tl;dr is that the APISelector's template changes the DOM tree structure just enough to break the way Bootstrap display's invalid feedback for fields. See my comment on the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the work-around for this one is to just use Django's built in Select widget, since with the HTMXSelect widget on the parent_object_type form field, the form is being rendered and a queryset attached to the parent form field select, we don't necessarily need the APISelect widget functionality.

The downside is that means no object selector modal for the parent form field for the time being.

Alternatively, we solve #18916 real quick.

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussing this in the maintainers call, we decided to move forward with this work as is re: the lack of validation messaging working in the DynamicModelChoiceField. #18916 will be a follow up in a future patch release.

@jnovinger jnovinger force-pushed the 8423-assign-service-to-fhrp-group branch from fa20cf6 to 385749c Compare April 10, 2025 19:48
@jnovinger
Copy link
Member Author

@arthanson , please note that the migrations have changed, for a couple of reasons:

  1. there was a conflict with a migration from another branch that got merged ahead of this one
  2. I had forgotten to enforce that Service.parent could not be null on the model itself.

You'll need at least wind back the previous IPAM 0078, 0079, and 0080 migrations before trying to run these.

@jnovinger jnovinger requested a review from arthanson April 10, 2025 20:08
@jnovinger
Allow assigning Service to FHRP Group, in addition to Device and Virt…
60e8268 
…ualMachine
@jnovinger
Finish up add FHRP Group assignment to Service
0564ee9 
- fixes up ServiceSerializer to support write operations
- fixes up GraphQL components: ServiceType and ServiceFilter
- fixes broken tests
- cleans up lint issues
@jnovinger
Remove unnecessary limit_choices_to arg in new Service.parent_object_…
158bb0d 
…type field

Re: #17166
@jnovinger
Address PR feedback, add FHRP Group to ServiceFilterForm
79e3067
jnovinger added 13 commits April 10, 2025 17:55
@jnovinger
Address PR feedback, clean up migrations
8cb2b8b
@jnovinger
Address PR feedback, add Services to FHRP Group related objects
37a2d90
@jnovinger
Address PR feedback, add ServiceFilterSet test for fhrpgroup
be3d55a
@jnovinger
Fix Service detail page breadcrumb links
048175f
@jnovinger
Address PR feedback, switch parent GFK to dropdown on forms
384e347 
a la ScopedForm
@jnovinger
Fix broken tests from changes to ServiceForm/ServiceCreateForm
189eda8
@jnovinger
Fixup multi-if to use match statement
0fdadd0
@jnovinger
Fixes up ServiceImportForm to work with new parent field
f53e310
@jnovinger
Raise ValidationError if parent object form data is incomplete
2bc636f
@jnovinger
Update docs to reflect change of Service.parent in v4.3
fbcbe27
@jnovinger
Address PR feedback, ensure that parent GFK underlying fields are not…
06aa7ea 
… nullable

This changes an existing (within the branch) migration, so if you're
testing this you'll need to back up migrations and re-run.
@jnovinger
Fix migration conflict after rebase on feature branch
6d13a54
@jnovinger
Fix up migration dependency after rebase
96a5b90
@jnovinger jnovinger force-pushed the 8423-assign-service-to-fhrp-group branch from 385749c to 96a5b90 Compare April 11, 2025 10:47
@arthanson arthanson merged commit f96df73 into feature Apr 11, 2025
6 checks passed
@jeremystretch jeremystretch deleted the 8423-assign-service-to-fhrp-group branch April 11, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arthanson arthanson arthanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jnovinger @arthanson @jeremystretch