[sled-agent-types] reorganize per RFD 619 #9488
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
This looks really good! I didn't review any of the moved code particularly carefully, assuming the compiler + dropshot-api-manager would catch any issues there. Almost all my comments are minor style / inconsistency things - only one is a question about a general rule (whether a *_vN API should refer to any latest::* types at all).
sled-agent/types/versions/src/add_dual_stack_shared_network_interfaces/inventory.rs
Outdated
Show resolved
Hide resolved
sled-agent/types/versions/src/add_dual_stack_shared_network_interfaces/probes.rs
Outdated
Show resolved
Hide resolved
| use omicron_uuid_kinds::VnicUuid; | ||
| use omicron_uuid_kinds::{BlueprintUuid, MupdateOverrideUuid}; | ||
| use omicron_uuid_kinds::{CollectionUuid, MupdateUuid}; | ||
| use sled_agent_types_versions::latest::inventory::ZoneKind; |
There was a problem hiding this comment.
I think I expected stuff like this to be using types from a "types" crate. Is this just because there's no need for a types crate for sled agent?
There was a problem hiding this comment.
I believe this is because nexus-sled-agent-shared went away and now nexus depends on this crate directly. The reason for nexus-sled-agent-shared was to prevent having to have a direct dependency on sled-agent-types because we weren't sure what direction that dep should go: nexus -> sled-agent or sled-agent -> nexus.
However, after looking at this, and our decision to use server side versioning and update nexus last, it seems that we should just have nexus depend directly on sled-agent-types.
There was a problem hiding this comment.
Yeah in this case that happened because nexus-sled-agent-shared went away. However I think you're right and this (and maybe other) cases should refer to sled-agent-types directly.
There was a problem hiding this comment.
Fixed up everything, and verified it with rg "use sled_agent_types_versions::latest": https://gist.github.com/sunshowers/0f979123db9ddf853e1e86195dadf168
| let extract_current_omicron_zones_config = |data: &str| { | ||
| let as_v9: OmicronZonesConfigV9 = serde_json::from_str(data).unwrap(); | ||
| OmicronZonesConfigV10::try_from(as_v9) | ||
| let as_v4: OmicronZonesConfigV4 = serde_json::from_str(data).unwrap(); |
There was a problem hiding this comment.
I think so, yeah, since OmicronZonesConfigV9 became OmicronZonesConfigV4.
| use omicron_uuid_kinds::VnicUuid; | ||
| use omicron_uuid_kinds::{BlueprintUuid, MupdateOverrideUuid}; | ||
| use omicron_uuid_kinds::{CollectionUuid, MupdateUuid}; | ||
| use sled_agent_types_versions::latest::inventory::ZoneKind; |
There was a problem hiding this comment.
I believe this is because nexus-sled-agent-shared went away and now nexus depends on this crate directly. The reason for nexus-sled-agent-shared was to prevent having to have a direct dependency on sled-agent-types because we weren't sure what direction that dep should go: nexus -> sled-agent or sled-agent -> nexus.
However, after looking at this, and our decision to use server side versioning and update nexus last, it seems that we should just have nexus depend directly on sled-agent-types.
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
| #[path = "initial/mod.rs"] | ||
| pub mod v1; | ||
| #[path = "add_dual_stack_shared_network_interfaces/mod.rs"] | ||
| pub mod v10; |
There was a problem hiding this comment.
Nit: What if instead of auto-sorting these we ensured manual sorting and left a space between them? Would that break merge conflict detection?
There was a problem hiding this comment.
I mention this, because not being in version order is strange to look at for a human.
There was a problem hiding this comment.
definitely, but I think making a mistake with manual sorting (no merge conflict) has a lot of downside and looking a bit weird has less of one.
sunshowers
deleted the
sunshowers/spr/rfc-sled-agent-types-reorganize-per-rfd-619
branch
sunshowers
changed the title
In #9488 we moved versioned Sled Agent types to the sled-agent-types-versions crate. In that PR we moved functional code into that crate as well, living in the same files as types. However, dealing with updating this code can easily go wrong, particularly around merge conflicts. Move this kind of functional code to an `impls` module, so that it is always implemented on the latest versions of types.
Reorganize sled-agent-types based on the model described in RFD 619. Putting this up for early feedback.