client: prepare instance spec types for use in sled-agent API #896
Conversation
The next step toward implementing virtual platforms in Omicron is to move instance spec construction from sled-agent up to Nexus. This requires instance spec types to appear in sled-agent's API. To allow this: - Direct the generated client to add a `schemars::JsonSchema` derive to all generated types. - Add `rename = snake_case` attributes to instance spec enum types that sled-agent expects to reuse. Omicron requires all of its API specifications to pass the OpenAPI linting rules defined in the openapi-lint crate (https://github.com/oxidecomputer/openapi-lint); among these is a requirement that enum variants all have snake case names. Tests: cargo test; PHD; verified that it's possible to write the desired Omicron changes using this change; verified that the Falcon repo continues to build with its propolis-client dependency pointed to this commit.
|
The Also, for the record: I looked into writing a test that runs the OpenAPI linter over the entire propolis-server API document (instead of just cherry-picking fixes for errors in sled-agent). This test is trivial to write, but it immediately turns up several dozen errors that we'd have to go fix, so I've punted that to another PR. (I may come to regret this as I keep working on my sled-agent changes...) |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
I had one thought about potentially changing the schema for SpecKey, but otherwise, this seems good to me!
| // WARNING: Progenitor-generated versions of this type will use serde's default | ||
| // derived Serialize and Deserialize implementations, not the serde_with | ||
| // implementations. This can cause clients to serialize spec keys in a form that | ||
| // the server can't deserialize. To get around this, Progenitor clients that | ||
| // use spec keys should use a `replace` directive to replace the generated | ||
| // type with this type. |
There was a problem hiding this comment.
It occurs to me that there might be an alternative solution to this that doesn't require "WARNING: remember to do this extra stuff when generating clients": we are presently deriving JsonSchema, and I assume the generated schema is for a tagged enum representation. Could we, instead, add serde(untagged) and hand-implement JsonSchema for SpecKey, the way we do for NameOrId in omicron-common? That way, I think we should instead generate an OpenAPI spec that represents this as just a String but with hints suggesting it could be a UUID. I'm not 100% sure that will get us the desired behavior but it seems worth looking into.
There was a problem hiding this comment.
I am embarrassed to admit that I did not see the parallel between this and NameOrId! I'll give this a try.
There was a problem hiding this comment.
I was able to get this to work in fc7a85f. It didn't exactly spark joy, but it does get rid of the serde_with dependency and makes things slightly nicer for sled-agent, so I think I'm leaning toward going with it anyway. Would be interested to hear what you think!
This obviates the need to use re-exports and Progenitor replace directives in sled-agent's API.
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
The untagged enum seems fine to me. The "order matters" comment is certainly a bit sad, but I think if we added a test for it we could safely just forget about it and be fine. Everything else seems good.
The next step toward implementing virtual platforms in Omicron is to move instance spec construction from sled-agent up to Nexus. This requires instance spec types to appear in sled-agent's API. To allow this:
schemars::JsonSchemaderive to all generated types.rename = snake_caseattributes to instance spec enum types that sled-agent expects to reuse. Omicron requires all of its API specifications to pass the OpenAPI linting rules defined in the openapi-lint crate (https://github.com/oxidecomputer/openapi-lint); among these is a requirement that enum variants all have snake case names.Also add a note explaining special rules for the use of the
SpecKeytype (generated clients that include this type need to make sure that they furnish the correct Serialize implementation for it).Tests: cargo test; PHD; verified that it's possible to write the desired Omicron changes using this change; verified that the Falcon repo continues to build with its propolis-client dependency pointed to this commit.