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

Split out mock propolis-server from real thing #556

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

pfmooney
Copy link
Collaborator

The simulated sled-agent in Omicron uses the Propolis mock server as a stand-in for a real VM instance. In order to break an awkward dependencies situation, it would be nice if the mock server did not haul in all everything which propolis-server requires.

Fixes #553

The simulated sled-agent in Omicron uses the Propolis mock server as a
stand-in for a real VM instance.  In order to break an awkward
dependencies situation, it would be nice if the mock server did not haul
in all everything which propolis-server requires.

Fixes oxidecomputer#553
Copy link
Contributor

@gjcolombo gjcolombo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one small tweak. Thanks for the changes!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If rust-analyzer is giving me an accurate picture of who's using things in this module, I would just drop it entirely: the only reason you need ServerSpecBuilderError is to serve as the return type to slot_to_pci_path, and in the mock, none of that function's callers actually care what error variant they got or about emitting their own ServerSpecBuilderError to a caller. So I'd just copy slot_to_pci_path to lib.rs, change its error type, and call it a day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're alright with that change being slightly delayed, my follow-on for this does clean up that error case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm suggesting something more radical than what's in 4919115: not only can we drop the InnerBuilderError variant from mock_server::copied::ServerSpecBuilderError, I suspect we can get rid of the entire enum and just have mock_server::copied::slot_to_pci_path return some other error type for its Err variant (at which point I'd probably just get rid of the copied module entirely and just move the private copy of slot_to_pci_path to the lib).

All that said, this is just cleanup and aesthetics, no need to hold this particular PR for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're happy with clean-up like that happening later, then I'm inclined to move forward with things as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we can tackle that in a later PR.

@pfmooney pfmooney merged commit 5a8b782 into oxidecomputer:master Nov 8, 2023
10 checks passed
@pfmooney pfmooney deleted the mock-split branch November 8, 2023 02:58
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.

Split mock-server from propolis-server
2 participants