-
Notifications
You must be signed in to change notification settings - Fork 45
Generate clients from openapi documents #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This looks great.
I think this fixes the extra |
Thank you!
Yes, I think that may be right. It was a lot to keep in my head, so I tried to take the approach that had fewer moving pieces, but agreed that it may get simpler without extraordinary efforts. |
@@ -1,168 +0,0 @@ | |||
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 I'm pretty excited about this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to make my comment an approval
@@ -319,16 +328,17 @@ impl Nexus { | |||
&self, | |||
id: Uuid, | |||
address: SocketAddr, | |||
) -> OximeterClient { | |||
) -> (OximeterClient, Uuid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why this returns the id, since the caller already has it.
I opened a new PR, but note the comments in #315
This replaces hand-rolled clients with ones generated via a macro from https://github.com/oxidecomputer/progenitor. Progenitor ingests OpenAPI documents and emits Rust code (in this case, we're doing it via the
generate_api!
macro, but a build.rs version exists as well). Progenitor uses https://github.com/oxidecomputer/typify to do the JSON Schema to Rust code conversion (progenitor first converts OpenAPI type schemas to JSON schema which are similar but different).Mostly, I think this is an improvement. The area where it's most obviously a step in the wrong direction is that for a given type
X
we now have potentially several versions of that type:X
as we define it in Rust and then one from each generated client. Accordingly, I wrote (script-assisted)From
implementations to convert. There are a couple of ways I'd like to take this forward in subsequent PRs:use
those types. This opens the door for tricky-to-debug disagreements between client and server.Another area that needs improvement is error types and handling. Right now progenitor uses anyhow which really isn't appropriate for this use. I'll replace that with a better Error type. The previous HTTP client assumed (correctly) that servers respond with our serialized Error type (
omicron/common/src/http_client.rs
Line 106 in 488db25
There was a divergence between the name of client methods and the server methods (operationId). I chose to modify the client names because I was already modifying the clients. For example:
My personal preference is to have the contents of the OpenAPI doc (and therefore the endpoint function names) be more semantics-oriented rather than path/protocol-oriented, but it's a reasonable position that that obfuscates important details.