Adapt to LLDP-related changes in link APIs #808
Conversation
|
Thanks @Nieuwejaar, just one minor suggestion. |
|
Since multiple changes need to be picked up here, maybe it would be simplest just to not do the bgp suff here and have this PR as a branch from |
I guess I can just wait for you to push that, and I'll rebase this on top of it. |
| "chassis_id": { | ||
| "nullable": true, | ||
| "description": "The LLDP chassis identifier TLV.", | ||
| "type": "string" | ||
| }, | ||
| "enabled": { | ||
| "description": "Whether or not LLDP is enabled.", | ||
| "type": "boolean" | ||
| }, | ||
| "lldp_config": { | ||
| "link_description": { | ||
| "nullable": true, | ||
| "description": "A reference to the LLDP configuration used. Must not be `None` when `enabled` is `true`.", | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/NameOrId" | ||
| } | ||
| ] | ||
| "description": "The LLDP link description TLV.", | ||
| "type": "string" | ||
| }, | ||
| "link_name": { | ||
| "nullable": true, | ||
| "description": "The LLDP link name TLV.", | ||
| "type": "string" | ||
| }, | ||
| "management_ip": { | ||
| "nullable": true, | ||
| "description": "The LLDP management IP TLV.", | ||
| "type": "string", | ||
| "format": "ip" | ||
| }, | ||
| "system_description": { | ||
| "nullable": true, | ||
| "description": "The LLDP system description TLV.", | ||
| "type": "string" | ||
| }, | ||
| "system_name": { | ||
| "nullable": true, | ||
| "description": "The LLDP system name TLV.", | ||
| "type": "string" |
There was a problem hiding this comment.
I think this kind of documentation isn't clarifying or helpful to users; I'd suggest we either delete it or replace it with something more explanatory.
There was a problem hiding this comment.
Agreed. None of this is really meant to be exposed to users yet, as it isn't plumbed through on the back end, but it got swept up in the auto-generated API. My hope/expectation is that this part of the LLDP support will be part of R11.
| speed: self.speed, | ||
| //TODO not fully plumbed on the back end yet. | ||
| lldp: LldpServiceConfigCreate { | ||
| lldp: LldpLinkConfigCreate { |
There was a problem hiding this comment.
perhaps lldp doesn't need to be a required field?
There was a problem hiding this comment.
No, it doesn't. We can easily construct a disabled Default on the server side.
|
Closing, as this has been subsumed by #813. |
The external
omicronAPIs related to link creation changed recently, breaking the CLI. This PR just adapts to those changes to restore the previous functionality, but does not add and LLDP-related functionality.Since the last CLI update, a BGP-related API was also added. I think I've got that plumbed in as well.
The link-related changed were tested by @elaine-oxide in a4x2.
I've added Ry as a reviewer to verify the BGP-related stuff, and Will because he's been by far the most active contributor recently. If there is somebody else I should pull in, please let me know.