[crucible-downstairs] migrate to API traits #1768
Conversation
Created using spr 1.3.6-beta.1
downstairs-types/src/lib.rs
Outdated
| // Repair API types | ||
| #[derive(Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct Eid { |
There was a problem hiding this comment.
Is this needed any more? It looks like it was replaced with ExtentPath in extent_repair_ready and get_files_for_extent?
There was a problem hiding this comment.
Ah yeah -- would you prefer I rename this back to Eid or keep it as ExtentPath?
There was a problem hiding this comment.
Either works, whatever matches our convention - ExtentPath has less of a naming collision than Eid haha.
Cargo.toml
Outdated
| crucible-downstairs-admin-api = { path = "./downstairs-admin-api" } | ||
| crucible-downstairs-repair-api = { path = "./downstairs-repair-api" } |
There was a problem hiding this comment.
Why two separate API traits here? They're both for the Downstairs, and (I think?) should be versioned together.
There was a problem hiding this comment.
Hmm, so both could live in the same crate, I thought you'd prefer the opposite haha.
The reason I did this is that crucible-downstairs-repair's OpenAPI document is checked into crucible, but the crucible-downstairs-admin document is not checked in. I figured that separating the two makes the most sense because they have different characteristics. But if you prefer they live in the same crate that's fine too.
There was a problem hiding this comment.
I haven't had my head in this space but I think they should live in the same crate, it seems easier to map downstairs version to one api version instead of two separate ones.
It might be an oversight to not have checked in the admin document, but: if both traits are combined that'll mean that the combined document should cover both right?
There was a problem hiding this comment.
It might be an oversight to not have checked in the admin document, but: if both traits are combined that'll mean that the combined document should cover both right?
So yes, but at the moment it isn't possible to combine multiple traits into one document -- this is work that we deferred, there isn't anything fundamental about traits that makes them impossible to combine.
Created using spr 1.3.6-beta.1
sunshowers
deleted the
sunshowers/spr/crucible-downstairs-migrate-to-api-traits
branch
2 participants
Migrate the downstairs admin and repair APIs to being API traits.
Part of oxidecomputer/omicron#8922.