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

[1/n] [reconfigurator-cli] split into binary/library #7892

Merged

Conversation

sunshowers
Copy link
Contributor

This split allows other crates to depend on reconfigurator-cli and use it in tests. We're going to introduce such a test in the next commit.

Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title [reconfigurator-cli] split into binary/library [1/n] [reconfigurator-cli] split into binary/library Mar 29, 2025
Created using spr 1.3.6-beta.1
@davepacheco
Copy link
Collaborator

I was a little surprised this looks like it only exposes CmdReconfiguratorSim and its exec method. This isn't what I'd think of as a library interface to what reconfigurator-cli does. But that's probably good because in principle it seems like we shouldn't need one -- I'd expect it'd be a pretty thin wrapper around existing library interfaces.

So this is just a way of invoking the command itself without having to fork/exec it? It looks like you can't even specify the arguments programmatically (since they're non-pub) so you must still be relying on clap to parse them out of an array of strings?

This seems fine. I'm just trying to understand the motivation and I'm wondering if we should document it somehow.


Edit: I see from the follow-on PR that this is just so that we can create a second reconfigurator-cli binary, exactly the same as the first, and solely so that we can use it in the integration test from another package, and only because Cargo doesn't let us use the binary from this package. Ugh, this seems like a lot of work/complexity, but I guess faster iteration on this package is worth it?

@sunshowers
Copy link
Contributor Author

This isn't what I'd think of as a library interface to what reconfigurator-cli does.

Yeah, I'd treat the reconfigurator sim as the true library here and this as a command interpreter/REPL on top of that.

So this is just a way of invoking the command itself without having to fork/exec it? It looks like you can't even specify the arguments programmatically (since they're non-pub) so you must still be relying on clap to parse them out of an array of strings?

For now, yes -- though we could just as well allow more programmatic command specification in the future.

Edit: I see from the follow-on PR that this is just so that we can create a second reconfigurator-cli binary, exactly the same as the first, and solely so that we can use it in the integration test from another package, and only because Cargo doesn't let us use the binary from this package. Ugh, this seems like a lot of work/complexity, but I guess faster iteration on this package is worth it?

Yeah -- I was hoping to avoid making a whole binary and just use reconfigurator-cli in-process, but that would require that reconfigurator-cli not directly produce output to stdout/stderr -- which is something we can do in the future but we're skipping for now.

@sunshowers
Copy link
Contributor Author

This seems fine. I'm just trying to understand the motivation and I'm wondering if we should document it somehow.

Do you feel like the documentation in this block is sufficient?
https://github.com/oxidecomputer/omicron/pull/7893/files/d9090f896015d6f097cd825ad9a4e6693680c256#r2033913740

@sunshowers sunshowers merged commit 7a68b87 into main Apr 8, 2025
16 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/reconfigurator-cli-split-into-binarylibrary branch April 8, 2025 20:43
sunshowers added a commit that referenced this pull request Apr 9, 2025
…ther crate (#7893)

Decouple the reconfigurator-cli tests from this more expensive Nexus
integration test. The integration test is quite valuable to keep around,
but in my experience it fails much less often than the blueprint
snapshot-based tests, and having to build all of Nexus to run snapshot
tests is bothersome.

Depends on:

* #7892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants