Skip to content
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

More explicit guidance on updating auto-generated client bindings? #396

Closed
smklein opened this issue Nov 16, 2021 · 2 comments · Fixed by #406
Closed

More explicit guidance on updating auto-generated client bindings? #396

smklein opened this issue Nov 16, 2021 · 2 comments · Fixed by #406
Assignees
Labels
api Related to the API. documentation Improvements or additions to documentation. openapi Related to the OpenAPI specification and implementations.

Comments

@smklein
Copy link
Collaborator

smklein commented Nov 16, 2021

I'm making use of #369 , and it's pretty great. I do have a question though - do we have advice on "how to generate the APIs when the structures change"?

Here's the loop I ran into while developing locally...

  • I tried changing a structure being used in Nexus' API
  • This resulted in a compilation error, because the type was misaligned with the "nexus client" auto-generated structure
  • I tried manually modifying the nexus-internal.json file (I presume it can also be autogenerated via tests, but Nexus wasn't compiling, so those weren't compiling). This didn't seem to update the client.
  • I eventually made a manual modification to omicron/common/src/nexus_client.rs - which contains the generate_logging_api! call - to force recompilation. I don't think rust considers the nexus-internal.json file a dependency on the build graph. This finally seemed to fix the client.

So I have some follow-up questions as a result of this process:

  1. How do we recommend folks update the client bindings, when changes to those structs often result in compilation errors? Is there a specific ordering that needs to be followed? Should the JSON files be edited manually?
  2. Do we consider it a problem that "updating the JSON file doesn't actually cause re-compilation"? I was a little surprised that it isn't part of the build graph, but maybe this is working-as-expected.
  3. Could we add documentation for the recommended way to update these bindings?

I'm happy to help with this process any way I can!

@smklein smklein added documentation Improvements or additions to documentation. api Related to the API. openapi Related to the OpenAPI specification and implementations. labels Nov 16, 2021
@smklein
Copy link
Collaborator Author

smklein commented Nov 16, 2021

As discussed in the Nov 16th Control Plane huddle, there actually should be a workaround that does not involve updating JSON manually:

  1. Update the bindings in the Dropshot server, and only build that package using cargo build -p ....
  2. If the server can be built, the openapi option can be passed to regenerate JSON files (alternately, the "command" tests which make this invocation can also be executed).
  3. I think the client file calling generate_logging_api still needs to be manually poked, but it can then be re-built, to consume the new JSON bindings and produce new bindings.
  4. Then the client packages can now be built, using the new auto-generated bindings.

@ahl
Copy link
Contributor

ahl commented Nov 16, 2021

following up on this, I'm going to:

  1. fix the bug in which progenitor doesn't properly update when the given json file is modified
  2. document a prescribed methodology (as noted above)
  3. also document areas that may be harder to resolve but that seem less typical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the API. documentation Improvements or additions to documentation. openapi Related to the OpenAPI specification and implementations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants