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

refactor: move config TOML parsing into a submodule #737

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

gjcolombo
Copy link
Contributor

(Part 2 in the instance spec refactoring series; see #735.)

Move large parts of the config TOML parsing logic in server spec builder into their own module. The parsing logic is functionally the same; the main difference is that there's a new, more descriptive error type (ConfigTomlError) that converts to a ServerSpecBuilderError variant instead of a variant that just takes a String. The idea is eventually to have separate modules for parsing config TOMLs, InstanceEnsureRequests, and VersionedInstanceSpecs; this is the first small step in that direction.

Tests: PHD, cargo test, ad hoc propolis-server testing; launched a Falcon topology and verified by hitting the /instance/spec endpoint that it created all the correct devices.

@gjcolombo gjcolombo requested a review from hawkw August 1, 2024 21:17
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

overall, this looks good to me. i had some small suggestions about the code that was already here, but feel free to disregard me since that code was already here, and i understand not wanting to mess with it in a diff that mostly just moves things around.

bin/propolis-server/src/lib/spec/config_toml.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/config_toml.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/config_toml.rs Outdated Show resolved Hide resolved
@gjcolombo gjcolombo force-pushed the gjcolombo/instance-spec-rework/part-2 branch from fa36fb2 to f98e0ea Compare August 9, 2024 23:30
@gjcolombo
Copy link
Contributor Author

Will merge this and push part 3 first thing Monday. Thanks as always for the review, @hawkw!

@gjcolombo gjcolombo merged commit 9edd439 into master Aug 12, 2024
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/instance-spec-rework/part-2 branch August 12, 2024 15:54
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