Skip to content

Commit

Permalink
fix(cli): Make --simulate a global arg and enable it for all subcommands
Browse files Browse the repository at this point in the history
  • Loading branch information
@sasa-tomic
sasa-tomic committed Jul 10, 2023
1 parent 193b45a commit 91aaf0b
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 96 deletions.
33 changes: 16 additions & 17 deletions rs/cli/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,30 @@ use ic_management_types::Network;
#[derive(Parser, Clone)]
#[clap(about, version, author)]
pub struct Opts {
#[clap(long, env = "HSM_PIN")]
#[clap(long, env = "HSM_PIN", global = true)]
pub(crate) hsm_pin: Option<String>,
#[clap(long, value_parser=maybe_hex::<u64>, env = "HSM_SLOT")]
#[clap(long, value_parser=maybe_hex::<u64>, env = "HSM_SLOT", global = true)]
pub(crate) hsm_slot: Option<u64>,
#[clap(long, env = "HSM_KEY_ID")]
#[clap(long, env = "HSM_KEY_ID", global = true)]
pub(crate) hsm_key_id: Option<String>,
#[clap(long, env = "PRIVATE_KEY_PEM")]
#[clap(long, env = "PRIVATE_KEY_PEM", global = true)]
pub(crate) private_key_pem: Option<String>,
#[clap(long, env = "NEURON_ID")]
#[clap(long, env = "NEURON_ID", global = true)]
pub(crate) neuron_id: Option<u64>,
#[clap(long, env = "IC_ADMIN")]
#[clap(long, env = "IC_ADMIN", global = true)]
pub(crate) ic_admin: Option<String>,
#[clap(long, env = "DEV")]
#[clap(long, env = "DEV", global = true)]
pub(crate) dev: bool,
#[clap(short, long, env = "YES")]

// Skip the confirmation prompt
#[clap(short, long, env = "YES", global = true, conflicts_with = "simulate")]
pub(crate) yes: bool,
#[clap(long, env = "VERBOSE")]

// Simulate submission of the proposal, but do not actually submit it.
#[clap(long, aliases = ["dry-run", "dryrun", "no"], global = true, conflicts_with = "yes")]
pub(crate) simulate: bool,

#[clap(long, env = "VERBOSE", global = true)]
pub(crate) verbose: bool,

// Specify the target network. Should be either "mainnet" (default) or "staging".
Expand Down Expand Up @@ -210,10 +217,6 @@ pub(crate) mod version {
/// prevent unintended version in the future
#[clap(long)]
edit_summary: bool,

// Simulate submission of the proposal, but do not actually submit it.
#[clap(long)]
simulate: bool,
},

/// Bless replica version with release notes using the ic-admin CLI and
Expand All @@ -225,10 +228,6 @@ pub(crate) mod version {
/// Sepcify the name of the rc branch that contains the release
/// commits.
rc_branch_name: String,

// Simulate submission of the proposal, but do not actually submit it.
#[clap(long)]
simulate: bool,
},
}
}
Expand Down
85 changes: 25 additions & 60 deletions rs/cli/src/ic_admin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,13 @@ impl Cli {
);
}

pub(crate) fn propose_run(&self, cmd: ProposeCommand, opts: ProposeOptions) -> anyhow::Result<()> {
let exec = |cli: &Cli, cmd: ProposeCommand, opts: ProposeOptions| {
pub(crate) fn propose_run(&self, cmd: ProposeCommand, opts: ProposeOptions, simulate: bool) -> anyhow::Result<()> {
let exec = |cli: &Cli, cmd: ProposeCommand, opts: ProposeOptions, add_dryrun_arg: bool| {
cli.run(
&cmd.get_command_name(),
[
if opts.simulate && !cmd.args().contains(&String::from("--dry-run")) {
// Passthrough proposals may already contain `--dry-run`, which
// is equivalent to the ProposeOption `simulate`. ic-admin
// rejects double --dry-run flags, so we only add it when it
// is already not added.
// Make sure there is no more than one `--dry-run` argument, or else ic-admin will complain.
if add_dryrun_arg && !cmd.args().contains(&String::from("--dry-run")) {
vec!["--dry-run".to_string()]
} else {
Default::default()
Expand Down Expand Up @@ -167,60 +164,32 @@ impl Cli {
)
};

// Corner case -- a --help snuck into the ic-admin args.
// Only possible with passthrough proposals.
// Pass it through, and end the story here.
if cmd.args().contains(&String::from("--help")) {
return exec(self, cmd, opts);
// Simulated, or --help executions run immediately and do not proceed.
if simulate || cmd.args().contains(&String::from("--help")) || cmd.args().contains(&String::from("--dry-run")) {
return exec(self, cmd, opts, simulate);
}

let direct_execution = self.yes || opts.simulate;
if !direct_execution {
// User has not specified direct execution without confirmation,
// and has also not requested a dry-run. The default behavior
// is to first dry-run and then to confirm.

// Note how we make the dry_run bool passed to exec() false in case
// the ic-admin args already contains --dry-run, to avoid including
// the flag again since ic-admin takes it very poorly when the same
// flag is specified twice.
exec(
self,
cmd.clone(),
ProposeOptions {
simulate: true,
..opts.clone()
},
)?;
// If --yes was not specified, ask the user if they want to proceed
if !self.yes {
exec(self, cmd.clone(), opts.clone(), true)?;
}

// Last corner case. User has attempted to make a for-realz submission
// but the submission might fail in case there is no neuron detected.
// Bail out early with an error instead of attempting to run ic-admin,
// providing the user with direct actionable information on how to
// correct the issue and retry the proposal. We do this because ic-admin
// is very cryptic regarding informing the user of what failed in this
// failure case.
if self.neuron.is_none() && !opts.simulate {
return Err(anyhow::anyhow!("Submitting this proposal in non-simulation mode requires a neuron, which was not detected -- and will cause ic-admin to fail submitting the proposal. Please look through your scroll buffer for specific error messages about your HSM and address the issue preventing your neuron from being detected."));
// User wants to proceed but does not have neuron configuration. Bail out.
if self.neuron.is_none() {
return Err(anyhow::anyhow!("Submitting this proposal requires a neuron, which was not detected -- and will cause ic-admin to fail during submition. Please look through your scroll buffer for specific error messages about your HSM and address the issue that prevents your neuron from being detected."));
}

// Final execution. The user has committed to submitting the proposal.
// This may also be performed in simulation mode if the user specified
// --dry-run in the ic-admin slot for flags in the release_cli command
// line (for passthrough proposals) or the --simulate flag (for version
// proposals).
// https://www.youtube.com/watch?v=9jK-NcRmVcw#t=13
if !direct_execution
&& !Confirm::new()
.with_prompt("Do you want to continue?")
.default(false)
.interact()?
if Confirm::new()
.with_prompt("Do you want to continue?")
.default(false)
.interact()?
{
// Oh noes. User chickened out. Abort unconfirmed submission.
return Err(anyhow::anyhow!("Action aborted"));
// User confirmed the desire to submit the proposal and no obvious problems were
// found. Proceeding!
exec(self, cmd, opts, false)
} else {
Err(anyhow::anyhow!("Action aborted"))
}
exec(self, cmd, opts)
}

fn _run_ic_admin_with_args(&self, ic_admin_args: &[String], with_auth: bool) -> anyhow::Result<()> {
Expand Down Expand Up @@ -320,7 +289,7 @@ impl Cli {
}

/// Run an `ic-admin propose-to-*` command directly
pub(crate) fn run_passthrough_propose(&self, args: &[String]) -> anyhow::Result<()> {
pub(crate) fn run_passthrough_propose(&self, args: &[String], simulate: bool) -> anyhow::Result<()> {
if args.is_empty() {
println!("List of available ic-admin 'propose' sub-commands:\n");
for subcmd in self.grep_subcommands(r"\s+propose-to-(.+?)\s") {
Expand All @@ -347,11 +316,8 @@ impl Cli {
command: args[0].clone(),
args: args.iter().skip(1).cloned().collect::<Vec<_>>(),
};
let opts = ProposeOptions {
simulate: cmd.args().contains(&String::from("--dry-run")),
..Default::default()
};
self.propose_run(cmd, opts)
let simulate = simulate || cmd.args().contains(&String::from("--dry-run"));
self.propose_run(cmd, Default::default(), simulate)
}

pub(crate) async fn prepare_to_propose_to_bless_new_replica_version(
Expand Down Expand Up @@ -596,7 +562,6 @@ pub struct ProposeOptions {
pub title: Option<String>,
pub summary: Option<String>,
pub motivation: Option<String>,
pub simulate: bool,
}

fn detect_hsm_auth() -> Result<Option<Auth>> {
Expand Down
23 changes: 12 additions & 11 deletions rs/cli/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ async fn main() -> Result<(), anyhow::Error> {
cli_opts.neuron_id = Some(STAGING_NEURON_ID);
}

let simulate = cli_opts.simulate;

match &cli_opts.subcommand {
cli::Commands::DerToPrincipal { path } => {
let principal = ic_base_types::PrincipalId::new_self_authenticating(&std::fs::read(path)?);
Expand Down Expand Up @@ -69,7 +71,7 @@ async fn main() -> Result<(), anyhow::Error> {
}

match &subnet.subcommand {
cli::subnet::Commands::Deploy { version } => runner.deploy(&subnet.id.unwrap(), version),
cli::subnet::Commands::Deploy { version } => runner.deploy(&subnet.id.unwrap(), version, simulate),
cli::subnet::Commands::Replace {
nodes,
no_heal,
Expand Down Expand Up @@ -110,7 +112,7 @@ async fn main() -> Result<(), anyhow::Error> {
only: only.clone(),
include: include.clone().into(),
min_nakamoto_coefficients,
}, *verbose)
}, *verbose, simulate)
.await
}
cli::subnet::Commands::Resize { add, remove, include, only, exclude, motivation, verbose, } => {
Expand All @@ -122,7 +124,7 @@ async fn main() -> Result<(), anyhow::Error> {
only: only.clone().into(),
exclude: exclude.clone().into(),
include: include.clone().into(),
}, motivation, *verbose).await
}, motivation, *verbose, simulate).await
} else {
cmd.error(
ErrorKind::MissingRequiredArgument,
Expand All @@ -140,7 +142,7 @@ async fn main() -> Result<(), anyhow::Error> {
only: only.clone().into(),
exclude: exclude.clone().into(),
include: include.clone().into(),
}, motivation, *verbose, replica_version.clone()).await
}, motivation, *verbose, simulate, replica_version.clone()).await
} else {
cmd.error(
ErrorKind::MissingRequiredArgument,
Expand All @@ -159,12 +161,12 @@ async fn main() -> Result<(), anyhow::Error> {

cli::Commands::Propose { args } => {
let ic_admin = ic_admin::Cli::from_opts(&cli_opts, true).await?;
ic_admin.run_passthrough_propose(args)
ic_admin.run_passthrough_propose(args, simulate)
},

cli::Commands::Version(cmd) => {
match &cmd.subcommand {
Retire { edit_summary , simulate} => {
Retire { edit_summary } => {
let runner = runner::Runner::from_opts(&cli_opts).await?;
let (template, versions) = runner.prepare_versions_to_retire(*edit_summary).await?;
let ic_admin = ic_admin::Cli::from_opts(&cli_opts, true).await?;
Expand All @@ -174,11 +176,11 @@ async fn main() -> Result<(), anyhow::Error> {
title: Some("Retire IC replica version".to_string()),
summary: Some(template),
motivation: None,
simulate: *simulate,
},
simulate,
)
},
Update { version, rc_branch_name ,simulate} => {
Update { version, rc_branch_name } => {
let runner = runner::Runner::from_opts(&cli_opts).await?;
let (_, versions) = runner.prepare_versions_to_retire(false).await?;
let ic_admin = ic_admin::Cli::from_opts(&cli_opts, true).await?;
Expand All @@ -198,8 +200,7 @@ async fn main() -> Result<(), anyhow::Error> {
title: proposal_title,
summary: Some(new_replica_info.summary),
motivation: None,
simulate: *simulate,
})
}, simulate)
}
}
},
Expand All @@ -219,7 +220,7 @@ async fn main() -> Result<(), anyhow::Error> {
extra_nodes_filter: extra_nodes_filter.clone(),
no_auto: *no_auto,
motivation: motivation.clone().unwrap_or_default(),
}).await
}, simulate).await
},
}
},
Expand Down
1 change: 0 additions & 1 deletion rs/cli/src/ops_subnet_node_replace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,5 @@ pub fn replace_proposal_options(change: &SubnetChangeResponse) -> anyhow::Result
title: format!("Replace {replace_target} in subnet {subnet_id_short}",).into(),
summary: format!("# Replace {replace_target} in subnet {subnet_id_short}",).into(),
motivation: change.motivation.clone(),
simulate: false,
})
}
Loading

0 comments on commit 91aaf0b

Please sign in to comment.