From 503b7fb93044f16a2deb330b8736685d31f29003 Mon Sep 17 00:00:00 2001 From: Manuel Amador Date: Wed, 19 Feb 2025 16:46:32 +0100 Subject: [PATCH] Move submitter code to separate module. --- rs/cli/src/commands/api_boundary_nodes/add.rs | 2 +- .../src/commands/api_boundary_nodes/remove.rs | 2 +- .../src/commands/api_boundary_nodes/update.rs | 2 +- rs/cli/src/commands/firewall.rs | 3 +- .../src/commands/governance/propose/motion.rs | 2 +- rs/cli/src/commands/hostos/rollout.rs | 2 +- .../hostos/rollout_from_node_group.rs | 2 +- rs/cli/src/commands/network.rs | 2 +- rs/cli/src/commands/nodes/remove.rs | 2 +- rs/cli/src/commands/propose.rs | 3 +- rs/cli/src/commands/subnet/create.rs | 2 +- rs/cli/src/commands/subnet/deploy.rs | 2 +- rs/cli/src/commands/subnet/replace.rs | 4 +- rs/cli/src/commands/subnet/rescue.rs | 2 +- rs/cli/src/commands/subnet/resize.rs | 2 +- .../src/commands/update_authorized_subnets.rs | 2 +- .../src/commands/update_unassigned_nodes.rs | 3 +- .../src/commands/version/revise/guest_os.rs | 2 +- rs/cli/src/commands/version/revise/host_os.rs | 2 +- rs/cli/src/forum/mod.rs | 80 ++----------------- rs/cli/src/lib.rs | 1 + rs/cli/src/main.rs | 1 + rs/cli/src/submitter.rs | 73 +++++++++++++++++ .../src/unit_tests/update_unassigned_nodes.rs | 3 +- rs/cli/src/unit_tests/version.rs | 3 +- 25 files changed, 107 insertions(+), 97 deletions(-) create mode 100644 rs/cli/src/submitter.rs diff --git a/rs/cli/src/commands/api_boundary_nodes/add.rs b/rs/cli/src/commands/api_boundary_nodes/add.rs index b24855a29..d095944d1 100644 --- a/rs/cli/src/commands/api_boundary_nodes/add.rs +++ b/rs/cli/src/commands/api_boundary_nodes/add.rs @@ -3,7 +3,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, ic_admin::{self}, }; diff --git a/rs/cli/src/commands/api_boundary_nodes/remove.rs b/rs/cli/src/commands/api_boundary_nodes/remove.rs index 26bafcd11..4bd732d90 100644 --- a/rs/cli/src/commands/api_boundary_nodes/remove.rs +++ b/rs/cli/src/commands/api_boundary_nodes/remove.rs @@ -3,7 +3,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, ic_admin::{self}, }; diff --git a/rs/cli/src/commands/api_boundary_nodes/update.rs b/rs/cli/src/commands/api_boundary_nodes/update.rs index 7460294b5..f02656e0e 100644 --- a/rs/cli/src/commands/api_boundary_nodes/update.rs +++ b/rs/cli/src/commands/api_boundary_nodes/update.rs @@ -3,7 +3,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, ic_admin::{self}, }; diff --git a/rs/cli/src/commands/firewall.rs b/rs/cli/src/commands/firewall.rs index 8024a5b7e..fb50568d8 100644 --- a/rs/cli/src/commands/firewall.rs +++ b/rs/cli/src/commands/firewall.rs @@ -14,9 +14,10 @@ use tempfile::NamedTempFile; use crate::{ ctx::DreContext, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::ForumPostKind, ic_admin::{IcAdminProposal, IcAdminProposalCommand, IcAdminProposalOptions}, proposal_executors::{ProducesProposalResult, ProposalResponseWithId, RunnableViaIcAdmin}, + submitter::{SubmissionParameters, Submitter}, }; use super::{AuthRequirement, ExecutableCommand}; diff --git a/rs/cli/src/commands/governance/propose/motion.rs b/rs/cli/src/commands/governance/propose/motion.rs index c2da08dd3..67ce42c59 100644 --- a/rs/cli/src/commands/governance/propose/motion.rs +++ b/rs/cli/src/commands/governance/propose/motion.rs @@ -6,7 +6,7 @@ use tokio::io::AsyncReadExt; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, util::{extract_title_and_text, utf8}, }; diff --git a/rs/cli/src/commands/hostos/rollout.rs b/rs/cli/src/commands/hostos/rollout.rs index 0c09b1fa8..06ed368f6 100644 --- a/rs/cli/src/commands/hostos/rollout.rs +++ b/rs/cli/src/commands/hostos/rollout.rs @@ -3,7 +3,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/hostos/rollout_from_node_group.rs b/rs/cli/src/commands/hostos/rollout_from_node_group.rs index 3cc6ee4e8..f682f6707 100644 --- a/rs/cli/src/commands/hostos/rollout_from_node_group.rs +++ b/rs/cli/src/commands/hostos/rollout_from_node_group.rs @@ -4,7 +4,7 @@ use clap::{Args, ValueEnum}; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, operations::hostos_rollout::{NodeGroupUpdate, NumberOfNodes}, }; diff --git a/rs/cli/src/commands/network.rs b/rs/cli/src/commands/network.rs index 820a298bc..9340840fb 100644 --- a/rs/cli/src/commands/network.rs +++ b/rs/cli/src/commands/network.rs @@ -3,7 +3,7 @@ use itertools::Itertools; use log::info; use crate::{ - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, ic_admin::IcAdminProposal, }; diff --git a/rs/cli/src/commands/nodes/remove.rs b/rs/cli/src/commands/nodes/remove.rs index 488207001..b4abf3214 100644 --- a/rs/cli/src/commands/nodes/remove.rs +++ b/rs/cli/src/commands/nodes/remove.rs @@ -3,7 +3,7 @@ use decentralization::subnets::NodesRemover; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/propose.rs b/rs/cli/src/commands/propose.rs index 5961a7623..da23176c2 100644 --- a/rs/cli/src/commands/propose.rs +++ b/rs/cli/src/commands/propose.rs @@ -1,8 +1,9 @@ use clap::{error::ErrorKind, Args}; use crate::{ - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::ForumPostKind, ic_admin::{IcAdminProposal, IcAdminProposalCommand}, + submitter::{SubmissionParameters, Submitter}, }; use super::{AuthRequirement, ExecutableCommand}; diff --git a/rs/cli/src/commands/subnet/create.rs b/rs/cli/src/commands/subnet/create.rs index ef62f4958..550088f78 100644 --- a/rs/cli/src/commands/subnet/create.rs +++ b/rs/cli/src/commands/subnet/create.rs @@ -5,7 +5,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/subnet/deploy.rs b/rs/cli/src/commands/subnet/deploy.rs index 48b180cbd..79a3e116d 100644 --- a/rs/cli/src/commands/subnet/deploy.rs +++ b/rs/cli/src/commands/subnet/deploy.rs @@ -5,7 +5,7 @@ use ic_types::PrincipalId; use crate::{ auth::get_automation_neuron_default_path, commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/subnet/replace.rs b/rs/cli/src/commands/subnet/replace.rs index 57b914477..e0e46c761 100644 --- a/rs/cli/src/commands/subnet/replace.rs +++ b/rs/cli/src/commands/subnet/replace.rs @@ -3,10 +3,10 @@ use clap::{error::ErrorKind, Args}; use ic_types::PrincipalId; use itertools::Itertools; -use crate::forum::{ForumPostKind, SubmissionParameters}; +use crate::forum::ForumPostKind; +use crate::submitter::{SubmissionParameters, Submitter}; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::Submitter, subnet_manager::SubnetTarget, }; diff --git a/rs/cli/src/commands/subnet/rescue.rs b/rs/cli/src/commands/subnet/rescue.rs index 22aeda2b5..5fe289437 100644 --- a/rs/cli/src/commands/subnet/rescue.rs +++ b/rs/cli/src/commands/subnet/rescue.rs @@ -4,7 +4,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/subnet/resize.rs b/rs/cli/src/commands/subnet/resize.rs index 58e6d9704..5767eebfd 100644 --- a/rs/cli/src/commands/subnet/resize.rs +++ b/rs/cli/src/commands/subnet/resize.rs @@ -5,7 +5,7 @@ use ic_types::PrincipalId; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Args, Debug)] diff --git a/rs/cli/src/commands/update_authorized_subnets.rs b/rs/cli/src/commands/update_authorized_subnets.rs index de9999c75..31371d296 100644 --- a/rs/cli/src/commands/update_authorized_subnets.rs +++ b/rs/cli/src/commands/update_authorized_subnets.rs @@ -10,7 +10,7 @@ use itertools::Itertools; use log::info; use crate::{ - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, ic_admin::{IcAdminProposal, IcAdminProposalCommand, IcAdminProposalOptions}, }; diff --git a/rs/cli/src/commands/update_unassigned_nodes.rs b/rs/cli/src/commands/update_unassigned_nodes.rs index 45453ca34..86c477048 100644 --- a/rs/cli/src/commands/update_unassigned_nodes.rs +++ b/rs/cli/src/commands/update_unassigned_nodes.rs @@ -4,7 +4,8 @@ use clap::Args; use ic_canisters::registry::RegistryCanisterWrapper; use ic_types::PrincipalId; -use crate::forum::{ForumPostKind, SubmissionParameters, Submitter}; +use crate::forum::ForumPostKind; +use crate::submitter::{SubmissionParameters, Submitter}; use super::{AuthRequirement, ExecutableCommand}; diff --git a/rs/cli/src/commands/version/revise/guest_os.rs b/rs/cli/src/commands/version/revise/guest_os.rs index b81e68458..b5862c3c0 100644 --- a/rs/cli/src/commands/version/revise/guest_os.rs +++ b/rs/cli/src/commands/version/revise/guest_os.rs @@ -2,7 +2,7 @@ use clap::{error::ErrorKind, Args}; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Debug, Args)] diff --git a/rs/cli/src/commands/version/revise/host_os.rs b/rs/cli/src/commands/version/revise/host_os.rs index 61f8d8a1f..31a33dd0c 100644 --- a/rs/cli/src/commands/version/revise/host_os.rs +++ b/rs/cli/src/commands/version/revise/host_os.rs @@ -2,7 +2,7 @@ use clap::{error::ErrorKind, Args}; use crate::{ commands::{AuthRequirement, ExecutableCommand}, - forum::{ForumPostKind, SubmissionParameters, Submitter}, + forum::{ForumPostKind}, submitter::{SubmissionParameters, Submitter}, }; #[derive(Debug, Args)] diff --git a/rs/cli/src/forum/mod.rs b/rs/cli/src/forum/mod.rs index e1b1181c3..4b0ce8d43 100644 --- a/rs/cli/src/forum/mod.rs +++ b/rs/cli/src/forum/mod.rs @@ -3,19 +3,12 @@ use std::{path::PathBuf, str::FromStr}; use clap::Args as ClapArgs; use futures::future::BoxFuture; use ic_types::PrincipalId; -use log::warn; use mockall::automock; -use crate::{ - confirm::{ConfirmationModeOptions, HowToProceed}, - proposal_executors::ProposalExecution, - util::yesno, -}; - mod impls; #[derive(Debug, Clone)] -pub enum ForumPostLinkVariant { +pub(crate) enum ForumPostLinkVariant { Url(url::Url), ManageOnDiscourse, Ask, @@ -120,15 +113,6 @@ impl ForumParameters { } } -#[derive(ClapArgs, Debug, Clone)] -pub struct SubmissionParameters { - #[clap(flatten)] - pub forum_parameters: ForumParameters, - - #[clap(flatten)] - pub confirmation_mode: ConfirmationModeOptions, -} - // FIXME: this should become part of a new composite trait // that builds on the ProducesProposalResults trait, // so that we don't have to have a separate kind here, this just @@ -158,7 +142,7 @@ pub trait ForumPost: Sync + Send { } #[derive(Clone)] -struct ForumContext { +pub struct ForumContext { forum_opts: ForumParameters, } @@ -168,11 +152,6 @@ impl ForumContext { Self { forum_opts } } - // FIXME: turn into impl From. - fn from_opts(opts: &ForumParameters) -> Self { - Self::new(opts.clone()) - } - pub fn client(&self) -> anyhow::Result> { match &self.forum_opts.forum_post_link { ForumPostLinkVariant::Url(u) => Ok(Box::new(impls::UserSuppliedLink { url: Some(u.clone()) }) as Box), @@ -183,57 +162,8 @@ impl ForumContext { } } -/// Helps the caller preview and then submit a proposal automatically, -/// handling the forum post part of the work as smoothly as possible. -pub struct Submitter { - mode: HowToProceed, - forum_parameters: ForumParameters, -} - -impl From<&SubmissionParameters> for Submitter { - fn from(other: &SubmissionParameters) -> Self { - Self { - mode: (&other.confirmation_mode).into(), - forum_parameters: other.forum_parameters.clone(), - } - } -} - -impl Submitter { - /// Submits a proposal (maybe in dry-run mode) with confirmation from the user, unless the user - /// specifies in the command line that he wants no confirmation (--yes). - pub async fn propose(&self, execution: Box, kind: ForumPostKind) -> anyhow::Result<()> { - if let HowToProceed::Unconditional = self.mode { - } else { - execution.simulate().await?; - }; - - if let HowToProceed::Confirm = self.mode { - // Ask for confirmation - if !yesno("Do you want to continue?", false).await?? { - return Ok(()); - } - } - - if let HowToProceed::DryRun = self.mode { - Ok(()) - } else { - let forum_post = ForumContext::from_opts(&self.forum_parameters).client()?.forum_post(kind).await?; - let res = execution.submit(forum_post.url()).await; - match res { - Ok(res) => forum_post.add_proposal_url(res.into()).await, - Err(e) => { - if let Some(forum_post_url) = forum_post.url() { - // Here we would ask the forum post code to delete the post since - // the submission has failed... that is, if we had that feature. - warn!( - "Forum post {} may have been created for this proposal, but proposal submission failed. Please delete the forum post if necessary, as it now serves no purpose.", - forum_post_url - ); - }; - Err(e) - } - } - } +impl From<&ForumParameters> for ForumContext { + fn from(p: &ForumParameters) -> Self { + Self::new(p.clone()) } } diff --git a/rs/cli/src/lib.rs b/rs/cli/src/lib.rs index 23025031e..ae2480d1b 100644 --- a/rs/cli/src/lib.rs +++ b/rs/cli/src/lib.rs @@ -13,5 +13,6 @@ mod proposal_executors; mod qualification; mod runner; mod store; +mod submitter; mod subnet_manager; mod util; diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index 28a72c4fd..d7d3c27f6 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -21,6 +21,7 @@ mod proposal_executors; mod qualification; mod runner; mod store; +mod submitter; mod subnet_manager; #[cfg(test)] mod unit_tests; diff --git a/rs/cli/src/submitter.rs b/rs/cli/src/submitter.rs new file mode 100644 index 000000000..2da1127e2 --- /dev/null +++ b/rs/cli/src/submitter.rs @@ -0,0 +1,73 @@ +use clap::Args as ClapArgs; +use log::warn; + +use crate::{ + confirm::{ConfirmationModeOptions, HowToProceed}, + forum::{ForumContext, ForumParameters, ForumPostKind}, + proposal_executors::ProposalExecution, + util::yesno, +}; + +#[derive(ClapArgs, Debug, Clone)] +pub struct SubmissionParameters { + #[clap(flatten)] + pub forum_parameters: ForumParameters, + + #[clap(flatten)] + pub confirmation_mode: ConfirmationModeOptions, +} + +/// Helps the caller preview and then submit a proposal automatically, +/// handling the forum post part of the work as smoothly as possible. +pub struct Submitter { + mode: HowToProceed, + forum_parameters: ForumParameters, +} + +impl From<&SubmissionParameters> for Submitter { + fn from(other: &SubmissionParameters) -> Self { + Self { + mode: (&other.confirmation_mode).into(), + forum_parameters: other.forum_parameters.clone(), + } + } +} + +impl Submitter { + /// Submits a proposal (maybe in dry-run mode) with confirmation from the user, unless the user + /// specifies in the command line that he wants no confirmation (--yes). + pub async fn propose(&self, execution: Box, kind: ForumPostKind) -> anyhow::Result<()> { + if let HowToProceed::Unconditional = self.mode { + } else { + execution.simulate().await?; + }; + + if let HowToProceed::Confirm = self.mode { + // Ask for confirmation + if !yesno("Do you want to continue?", false).await?? { + return Ok(()); + } + } + + if let HowToProceed::DryRun = self.mode { + Ok(()) + } else { + let forum_post = ForumContext::from(&self.forum_parameters).client()?.forum_post(kind).await?; + let res = execution.submit(forum_post.url()).await; + match res { + Ok(res) => forum_post.add_proposal_url(res.into()).await, + Err(e) => { + if let Some(forum_post_url) = forum_post.url() { + // Here we would ask the forum post code to delete the post since + // the submission has failed... that is, if we had that feature. + warn!( + "Forum post {} may have been created for this proposal, but proposal submission failed. Please delete the forum post if necessary, as it now serves no purpose.", + forum_post_url + ); + }; + Err(e) + } + } + } + } +} diff --git a/rs/cli/src/unit_tests/update_unassigned_nodes.rs b/rs/cli/src/unit_tests/update_unassigned_nodes.rs index 1f24aad6b..65cbcfe7a 100644 --- a/rs/cli/src/unit_tests/update_unassigned_nodes.rs +++ b/rs/cli/src/unit_tests/update_unassigned_nodes.rs @@ -5,8 +5,9 @@ use crate::auth::Neuron; use crate::commands::{update_unassigned_nodes::UpdateUnassignedNodes, ExecutableCommand}; use crate::confirm::ConfirmationModeOptions; use crate::cordoned_feature_fetcher::MockCordonedFeatureFetcher; -use crate::forum::{ForumParameters, SubmissionParameters}; +use crate::forum::ForumParameters; use crate::ic_admin::MockIcAdmin; +use crate::submitter::SubmissionParameters; use ic_management_backend::health::MockHealthStatusQuerier; use ic_management_backend::{lazy_git::MockLazyGit, lazy_registry::MockLazyRegistry, proposal::MockProposalAgent}; use ic_management_types::{Network, Subnet}; diff --git a/rs/cli/src/unit_tests/version.rs b/rs/cli/src/unit_tests/version.rs index 792fba4de..bf7ea3ded 100644 --- a/rs/cli/src/unit_tests/version.rs +++ b/rs/cli/src/unit_tests/version.rs @@ -1,5 +1,6 @@ use crate::confirm::ConfirmationModeOptions; -use crate::forum::{ForumParameters, ForumPostLinkVariant, SubmissionParameters}; +use crate::forum::{ForumParameters, ForumPostLinkVariant}; +use crate::submitter::SubmissionParameters; use indexmap::IndexMap; use std::sync::{Arc, RwLock};