add x-rust-type to ArtifactHash
#6838
Conversation
|
Thanks for doing this! I personally like it but I'll let Dave accept it :) |
| @@ -328,6 +328,19 @@ pub fn hex_schema<const N: usize>(gen: &mut SchemaGenerator) -> Schema { | |||
| schema.into() | |||
| } | |||
|
|
|||
| fn artifact_hash_schema(gen: &mut SchemaGenerator) -> Schema { | |||
There was a problem hiding this comment.
might it be simpler to impl JsonSchema for ArtifactHash? And it might make sense to inline hex_schema, minimal as it is.
| @@ -328,6 +328,19 @@ pub fn hex_schema<const N: usize>(gen: &mut SchemaGenerator) -> Schema { | |||
| schema.into() | |||
There was a problem hiding this comment.
github doesn't let me comment on the code above, but this is a bit of a surprising construction. In particular the format field in JSON Schema / OpenAPI isn't generally used for free-form text guidance--it seems to be for specific, well-known formats such as email, uri, or uuid.
I would have expected this to be something more like:
{
"type": "string",
"pattern": "^[0-9a-fA-F]{64}",
}| let host_phase_2 = host_phase_2.unwrap(); | ||
| let control_plane = control_plane.unwrap(); |
There was a problem hiding this comment.
Do you happen to know why we import omicron_common::update::ArtifactHash for these rather than gateway_client::types::ArtifactHash?
| "x-rust-type".into(), | ||
| serde_json::json!({ | ||
| "crate": env!("CARGO_PKG_NAME"), | ||
| "version": env!("CARGO_PKG_VERSION"), |
There was a problem hiding this comment.
This doesn't seem quite right in that the version is intended to fixed at the version at which the type became public... which isn't particularly relevant here. Given the nature of the monorepo, I'd suggest
| "version": env!("CARGO_PKG_VERSION"), | |
| "version": "*", |
While working on #6764 I found myself partially wanting this.
I'm not the biggest fan of the particulars of this implementation but it does do what I want, even though
ArtifactHashis#[serde(transparent)]. This has the effect of moving theto_string()into the Progenitor-generated client, which can be nice if you're having a niche set of lifetime issues sinceArtifactHashisCopy. But because Progenitor implements this on a crate-by-crate basis, addingx-rust-typeto any additional types in omicron-common will mean making fixes across the tree wherever clients withcrates = { "omicron-common = "0.1.0 }are used.This is a sort of take-it-or-leave-it PR; it's here if we want it but I'd like to get consensus that we actually want it.