network API re-org trial #4276
network API re-org trial #4276
Conversation
|
As the original advocate for putting all the params together and all the views together (#191, #374) this is my official acknowledgment that that didn't really work out as we got too many unrelated things in those files, and this is better. Here's one small concern I have (though I see it doesn't come up much in the set of endpoints you've reorged here): you can see if you grep As we will probably not be changing the model side as part of reorganizing the views and params and endpoints, in places where we need to distinguish model and view, we can continue to qualify models with |
There was a problem hiding this comment.
I looked through in more detail and I approve! I especially like the addition of the conventional Create suffix to the end of types that represent POST bodies. It would be interesting if we could lint for that. This relates to my broader concern in the above comment, which animated the original ideas around organization, about making clear which types represent request bodies, response bodies, etc.
| api.register(networking_switch_port_settings_view)?; | ||
| api.register(networking_switch_port_settings_create)?; | ||
| api.register(networking_switch_port_settings_delete)?; | ||
| api.register(networking::networking_switch_port_settings_list)?; |
There was a problem hiding this comment.
I'm on-board with this refactor, we do something similar for the console
| @@ -2214,320 +2214,6 @@ pub struct LoopbackAddress { | |||
| pub address: IpNet, | |||
| } | |||
|
|
|||
| /// A switch port represents a physical external port on a rack switch. | |||
There was a problem hiding this comment.
I'm okay with moving this stuff into types/src/external_api, but I'd caution -- if we're going to do it for all types, we should plan on doing it pretty soon. Omicron suffers a bit from the syndrome of "we should do a refactor, but it's big, so we only got halfway through, and now things are a little more spread out than they were before".
However, if the goal is to chunk this up into submodules where things are related, I'm totally on-board, that seems like a win.
There was a problem hiding this comment.
Hopefully this one is easier because it's mostly grouping things within each crate. Last time I think the biggest difficulty I ran into was some types I couldn't untangle to move out of omicron-common.
There was a problem hiding this comment.
My hope for this is--if we like this direction--to as quickly as possible, dogpile this migration
| @@ -13663,6 +13700,43 @@ | |||
| "port_settings_id" | |||
| ] | |||
| }, | |||
| "SwitchPortBgpPeerConfigCreate": { | |||
There was a problem hiding this comment.
The prefixed names make sense here -- it's the sorta thing we'd want from uniquely-named submodules, but we're just doing explicitly.
|
FYI to both @rcgoodfellow and @internet-diglett -- up to y'all if you want to review, but you should be aware of this PR, since it'll likely cause some conflicts |
Is this on the docket for the upcoming release? |
I want to get this in ASAP, but obviously it's not as important as your BGP stuff. |
|
womp womp |
First, this is solving a problem: we have multiple networking types of the same name that are stomping on each other (until we merge #4266).
In addition, this is suggestion for how we might re-organize the API. Today, we have related things scattered between several files. This is an attempt to put like-things together. If this seems like a promising path, I'd suggest we merge this with the expectation that we do something similar for other parts of the API. If we hate it, fine, I'll redo this with just the type fixes.