-
Notifications
You must be signed in to change notification settings - Fork 9
Enhance VLAN validation and payload #69
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
Conversation
workflows/l2vpn/create_l2vpn.py
Outdated
| @step("Validate VLAN selection") | ||
| def validate_vlan_selection(ports: list[UUIDstr], vlan: VlanRanges) -> State: | ||
| info = SimpleNamespace(data={"ports": ports}) | ||
| validated_vlan = validate_vlan(vlan, info) | ||
| validate_vlan_not_in_use(validated_vlan, info, port_field_name="ports") | ||
| return {"ports": ports, "vlan": validated_vlan} |
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.
What does this validation add on top of the same validation in the form generator?
workflows/l2vpn/create_l2vpn.py
Outdated
| if sap.port.ims_id is None: | ||
| raise ValueError("Port IMS id missing; cannot associate VLANs") | ||
| sap.ims_id = sap.port.ims_id | ||
| vlan_payloads += build_sap_vlans_payload(sap, subscription) |
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.
For L2VPN the vlan groups should still be administered in IMS.
The "do not administrate VLAN group in Netbox, VLAN administration is only done in the orchestrator database" requirement only applies to NSISTP product/workflows (which is written above the summary, maybe that's the source of the confusion)
workflows/l2vpn/terminate_l2vpn.py
Outdated
| if vlan.vid in target_vlans: | ||
| netbox.delete_vlan(id=vlan.id) |
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.
Is this needed for a specific reason?
Mark90
left a comment
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.
LGTM
resolves #64