[2/n] [dpd] migrate to API trait #116
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
| #[derive(Debug, Deserialize, Serialize, JsonSchema)] | ||
| pub(crate) struct MulticastGroupCreateEntry { | ||
| group_ip: Ipv6Addr, | ||
| tag: Option<String>, | ||
| sources: Option<Vec<IpSrc>>, | ||
| members: Vec<MulticastGroupMember>, | ||
| } |
There was a problem hiding this comment.
I had to make these and other fields public. But note that they were effectively public via the Deserialize derive already, so this isn't any worse.
sunshowers
changed the base branch from
sunshowers/spr/main.2n-dpd-migrate-to-api-trait
to
main
Created using spr 1.3.6-beta.1
| #[allow(unused_mut)] | ||
| let mut api = dpd_api_mod::api_description::<DpdApiImpl>().unwrap(); | ||
|
|
||
| // TODO: need to move these into dpd-api |
There was a problem hiding this comment.
Am I reading this correctly that the API now has trait-based parts and non-trait based parts. E.g all the endpoints that were previously registered above are now in the trait, and the ASIC-specific endpoints below are still in the API but not in the trait?
There was a problem hiding this comment.
Correct, yeah. At the moment the function-based endpoints are part of the OpenAPI document (JSON file) though with the OpenAPI manager we'll need all endpoints present in the document to be moved into the trait. Hence the TODO.
There was a problem hiding this comment.
It's out of scope for this work, but I wonder if we can get rid of some (all?) of these exported "views" types now that we have a dpd-types crate that clients can import
There was a problem hiding this comment.
Possibly! Depends on if you want to maintain a separation between internal and public API types.
| //! Manage logical Ethernet links on the Sidecar switch. | ||
| use crate::api_server::LinkCreate; | ||
| use crate::fault; |
There was a problem hiding this comment.
If we have two modules with the same name (crate::fault and dpd_types::fault) being used in the same file, then it seems like we should make explicit which one we are using in each case. So, don't use crate::fault here. If we are going to choose one of the two modules to use, then I would use the internal facing crate::fault module and expand dpd_types::fault::XXX inline.
There was a problem hiding this comment.
To be clear... I don't have a style guide, or even necessarily prior art, to point at in support of this position. It's just what seems natural to me.
There was a problem hiding this comment.
Addressed by not using either ::fault, thanks.
There was a problem hiding this comment.
I just checked in a decent sized change to the route API, which I'm afraid has made some more work for you here. Sorry.
There was a problem hiding this comment.
No worries, updated!
Created using spr 1.3.6-beta.1
Part of oxidecomputer/omicron#8922. I had to do a lot of code movement in this PR, but there aren't any functional changes in it.
Depends on: