-
Notifications
You must be signed in to change notification settings - Fork 42
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
[6/n] [reconfigurator-cli] allow loading up an example system #6788
[6/n] [reconfigurator-cli] allow loading up an example system #6788
Conversation
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
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.
Looks great!
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.
Nice. Thanks. My suggestions here are mostly around clarifying naming and comments.
/// reset the state of the REPL | ||
Reset, |
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.
Can we give this a more descriptive name, like maybe clear
or wipe
?
When you say "reset the REPL" I think we're talking about terminal settings or something. What we're doing here is wiping all the in-memory state that describes the underlying system and making it match what you would get if you started the program again, right?
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.
Hah, good point re resetting terminal settings, I was only thinking about reset in the sense of a factory reset.
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.
Changed this to "wipe".
fn needs_reset_for_load_example(&self) -> bool { | ||
// Use this pattern to ensure that if a new field is added to | ||
// ReconfiguratorSim, it will fail to compile until it's added here. | ||
let Self { |
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.
This makes sense but I wonder if we can give more explicit instruction to somebody that's adding a new field? How do they know which category it's in?
All of this has me wondering if we want to separate out these pieces into a separate object that we replace wholesale when needed...but I don't have a concrete suggestion at this point. We can go with this and see how it goes.
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.
Yeah thinking about this, maybe we can introduce notions of a "system reset" (only system, not policy), a "policy reset", and a "full reset" (system and policy). Then to load an example, you'd need to do a system reset but not a policy reset.
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 started pulling this thread and it got out of hand a bit:
Cargo.lock | 16 +
Cargo.toml | 1 +
dev-tools/reconfigurator-cli/Cargo.toml | 3 +
dev-tools/reconfigurator-cli/src/dispatch.rs | 74 +++
dev-tools/reconfigurator-cli/src/lib.rs | 12 +
dev-tools/reconfigurator-cli/src/main.rs | 1310 +-------------------------------------------------
dev-tools/reconfigurator-cli/src/repl.rs | 1246 +++++++++++++++++++++++++++++++++++++++++++++++
dev-tools/reconfigurator-cli/src/rng.rs | 108 +++++
dev-tools/reconfigurator-cli/src/sim.rs | 346 +++++++++++++
dev-tools/reconfigurator-cli/tests/input/cmds-example.txt | 26 +
dev-tools/reconfigurator-cli/tests/output/cmd-example-stderr | 0
dev-tools/reconfigurator-cli/tests/output/cmd-example-stdout | 526 ++++++++++++++++++++
dev-tools/reconfigurator-cli/tests/test_basic.rs | 15 +
nexus/reconfigurator/planning/src/example.rs | 2 +-
nexus/reconfigurator/planning/src/system.rs | 12 +-
nexus/types/src/deployment/execution/dns.rs | 4 +-
16 files changed, 2390 insertions(+), 1311 deletions(-)
I'm going to tackle this in a separate followup PR.
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Add a new command to the reconfigurator CLI,
load-example
, that conjures upan example system. This allows for really easy setup, both interactively and
in automated tests. Plus, having everything be seeded is really nice for
reproducibility.
Depends on: