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

[6/n] [reconfigurator-cli] allow loading up an example system #6788

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Oct 6, 2024

Add a new command to the reconfigurator CLI, load-example, that conjures up
an 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:

Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
Copy link
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Collaborator

@davepacheco davepacheco left a 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.

dev-tools/reconfigurator-cli/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 437 to 438
/// reset the state of the REPL
Reset,
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to "wipe".

dev-tools/reconfigurator-cli/src/main.rs Outdated Show resolved Hide resolved
dev-tools/reconfigurator-cli/src/main.rs Outdated Show resolved Hide resolved
dev-tools/reconfigurator-cli/src/main.rs Outdated Show resolved Hide resolved
dev-tools/reconfigurator-cli/src/main.rs Outdated Show resolved Hide resolved
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 {
Copy link
Collaborator

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.

Copy link
Contributor Author

@sunshowers sunshowers Oct 8, 2024

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.

Copy link
Contributor Author

@sunshowers sunshowers Oct 11, 2024

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.

nexus/reconfigurator/planning/src/example.rs Outdated Show resolved Hide resolved
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the base branch from sunshowers/spr/main.6n-reconfigurator-cli-allow-loading-up-an-example-system to main October 11, 2024 18:18
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers enabled auto-merge (squash) October 11, 2024 18:27
@sunshowers sunshowers merged commit 3a3eb52 into main Oct 11, 2024
17 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/6n-reconfigurator-cli-allow-loading-up-an-example-system branch October 11, 2024 20:10
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.

3 participants