diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 4f764ff24..880fad561 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -39,13 +39,15 @@ jobs: ######################################## # Build and test + # Will run test as a local subprocess because for some tests + # create status files on certain locations (like $HOME) ######################################## - name: "🚀 Building" uses: ./.github/workflows/build with: GITHUB_TOKEN: "${{ secrets.GIX_CREATE_PR_PAT }}" - name: "🚀 Testing" - run: bazel test ... + run: bazel test --spawn_strategy=local ... # We don't need the linear-jira build and test step for now # - name: "🚀 Build and Test Linear-Jira with Bazel" diff --git a/rs/cli/src/commands/mod.rs b/rs/cli/src/commands/mod.rs index 7acac72eb..d0b4113df 100644 --- a/rs/cli/src/commands/mod.rs +++ b/rs/cli/src/commands/mod.rs @@ -20,6 +20,7 @@ use proposals::Proposals; use propose::Propose; use qualify::Qualify; use registry::Registry; +use strum::Display; use update_authorized_subnets::UpdateAuthorizedSubnets; use update_unassigned_nodes::UpdateUnassignedNodes; use upgrade::Upgrade; @@ -124,6 +125,13 @@ pub(crate) struct Args { #[clap(long, global = true, env = "IC_ADMIN")] pub ic_admin: Option, + #[clap(long, global = true, env = "IC_ADMIN_VERSION", default_value_t = IcAdminVersion::FromGovernance, value_parser = clap::value_parser!(IcAdminVersion), help = r#"Specify the version of ic admin to use +Options: + 1. from-governance, governance, govn, g => same as governance canister + 2. default, d => strict default version, embedded at build time + 3. => specific commit"#)] + pub ic_admin_version: IcAdminVersion, + /// To skip the confirmation prompt #[clap(short, long, global = true, env = "YES", conflicts_with = "dry_run")] pub yes: bool, @@ -299,3 +307,20 @@ pub enum IcAdminRequirement { Detect, // detect the neuron OverridableBy { network: Network, neuron: AuthNeuron }, // eg automation which we know where is placed } + +#[derive(Debug, Display, Clone)] +pub enum IcAdminVersion { + FromGovernance, + Default, + Strict(String), +} + +impl From<&str> for IcAdminVersion { + fn from(value: &str) -> Self { + match value { + "from-governance" | "governance" | "g" | "govn" => Self::FromGovernance, + "default" | "d" => Self::Default, + s => Self::Strict(s.to_string()), + } + } +} diff --git a/rs/cli/src/ctx.rs b/rs/cli/src/ctx.rs index 89b7c35d2..e95010eb9 100644 --- a/rs/cli/src/ctx.rs +++ b/rs/cli/src/ctx.rs @@ -16,13 +16,13 @@ use ic_management_backend::{ }; use ic_management_types::Network; use ic_registry_local_registry::LocalRegistry; -use log::info; +use log::{debug, info}; use url::Url; use crate::{ auth::{Auth, Neuron}, - commands::{Args, ExecutableCommand, IcAdminRequirement}, - ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl}, + commands::{Args, ExecutableCommand, IcAdminRequirement, IcAdminVersion}, + ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl, DEFAULT_IC_ADMIN_VERSION}, runner::Runner, subnet_manager::SubnetManager, }; @@ -55,6 +55,7 @@ impl DreContext { dry_run: bool, ic_admin_requirement: IcAdminRequirement, forum_post_link: Option, + ic_admin_version: IcAdminVersion, ) -> anyhow::Result { let network = match no_sync { false => ic_management_types::Network::new(network.clone(), &nns_urls) @@ -83,7 +84,8 @@ impl DreContext { (neuron_id, auth.clone()) }; - let (ic_admin, ic_admin_path) = Self::init_ic_admin(&network, neuron_id, auth_opts, yes, dry_run, ic_admin_requirement).await?; + let (ic_admin, ic_admin_path) = + Self::init_ic_admin(&network, neuron_id, auth_opts, yes, dry_run, ic_admin_requirement, ic_admin_version).await?; Ok(Self { proposal_agent: Arc::new(ProposalAgentImpl::new(&network.nns_urls)), @@ -112,6 +114,7 @@ impl DreContext { args.dry_run, args.subcommands.require_ic_admin(), args.forum_post_link.clone(), + args.ic_admin_version.clone(), ) .await } @@ -123,6 +126,7 @@ impl DreContext { proceed_without_confirmation: bool, dry_run: bool, requirement: IcAdminRequirement, + version: IcAdminVersion, ) -> anyhow::Result<(Option>, Option)> { if let IcAdminRequirement::None = requirement { return Ok((None, None)); @@ -147,18 +151,36 @@ impl DreContext { } } }; - let ic_admin_path = match should_update_ic_admin()? { - (true, _) => { - let govn_canister_version = governance_canister_version(network.get_nns_urls()).await?; - download_ic_admin(match govn_canister_version.stringified_hash.as_str() { - // Some testnets could have this version setup if deployed - // from HEAD of the branch they are created from - "0000000000000000000000000000000000000000" => None, - v => Some(v.to_owned()), - }) - .await? + + let ic_admin_path = match version { + IcAdminVersion::FromGovernance => match should_update_ic_admin()? { + (true, _) => { + let govn_canister_version = governance_canister_version(network.get_nns_urls()).await?; + debug!( + "Using ic-admin matching the version of governance canister, version: {}", + govn_canister_version.stringified_hash + ); + download_ic_admin(match govn_canister_version.stringified_hash.as_str() { + // Some testnets could have this version setup if deployed + // from HEAD of the branch they are created from + "0000000000000000000000000000000000000000" => None, + v => Some(v.to_owned()), + }) + .await? + } + (false, s) => { + debug!("Using cached ic-admin matching the version of governance canister, path: {}", s); + s + } + }, + IcAdminVersion::Default => { + debug!("Using default ic-admin, version: {}", DEFAULT_IC_ADMIN_VERSION); + download_ic_admin(None).await? + } + IcAdminVersion::Strict(ver) => { + debug!("Using ic-admin specified via args: {}", ver); + download_ic_admin(Some(ver)).await? } - (false, s) => s, }; let ic_admin = Some(Arc::new(IcAdminImpl::new( @@ -253,6 +275,11 @@ impl DreContext { pub fn forum_post_link(&self) -> Option { self.forum_post_link.clone() } + + #[cfg(test)] + pub fn ic_admin_path(&self) -> Option { + self.ic_admin_path.clone() + } } #[cfg(test)] diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index 43d46a686..490b94f51 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -767,7 +767,7 @@ pub struct ProposeOptions { pub motivation: Option, pub forum_post_link: Option, } -const DEFAULT_IC_ADMIN_VERSION: &str = "0ca139ca39dfee21c8ca75e7fe37422df65e4b96"; +pub const DEFAULT_IC_ADMIN_VERSION: &str = "0ca139ca39dfee21c8ca75e7fe37422df65e4b96"; fn get_ic_admin_revisions_dir() -> anyhow::Result { let dir = dirs::home_dir() @@ -817,7 +817,7 @@ pub async fn download_ic_admin(version: Option) -> Result { format!("https://download.dfinity.systems/ic/{version}/binaries/x86_64-linux/ic-admin.gz") }; info!("Downloading ic-admin version: {} from {}", version, url); - let body = reqwest::get(url).await?.bytes().await?; + let body = reqwest::get(url).await?.error_for_status()?.bytes().await?; let mut decoded = GzDecoder::new(body.as_ref()); let path_parent = path.parent().expect("path parent unwrap failed!"); diff --git a/rs/cli/src/unit_tests/ctx_init.rs b/rs/cli/src/unit_tests/ctx_init.rs new file mode 100644 index 000000000..ffa4714c7 --- /dev/null +++ b/rs/cli/src/unit_tests/ctx_init.rs @@ -0,0 +1,137 @@ +use std::path::PathBuf; + +use crate::auth::Auth; +use ic_canisters::governance::governance_canister_version; +use ic_management_types::Network; + +use crate::{commands::IcAdminVersion, ctx::DreContext, ic_admin::DEFAULT_IC_ADMIN_VERSION}; + +fn status_file_path() -> PathBuf { + dirs::home_dir().unwrap().join("bin").join("ic-admin.revisions").join("ic-admin.status") +} + +fn get_deleted_status_file() -> PathBuf { + let status_file = status_file_path(); + if status_file.exists() { + std::fs::remove_file(&status_file).unwrap() + } + status_file +} + +async fn get_context(network: &Network, version: IcAdminVersion) -> anyhow::Result { + DreContext::new( + network.name.clone(), + network.nns_urls.clone(), + Auth::Anonymous, + None, + false, + true, + false, + true, + crate::commands::IcAdminRequirement::Detect, + None, + version, + ) + .await +} + +struct TestScenario<'a> { + name: &'static str, + version: IcAdminVersion, + should_delete_status_file: bool, + should_contain: Option<&'a str>, +} + +impl<'a> TestScenario<'a> { + fn new(name: &'static str) -> Self { + Self { + name, + version: IcAdminVersion::FromGovernance, + should_delete_status_file: false, + should_contain: None, + } + } + + fn version(self, version: IcAdminVersion) -> Self { + Self { version, ..self } + } + + fn delete_status_file(self) -> Self { + Self { + should_delete_status_file: true, + ..self + } + } + + fn should_contain(self, ver: &'a str) -> Self { + Self { + should_contain: Some(ver), + ..self + } + } +} + +#[tokio::test] +async fn init_tests_ic_admin_version() { + let version_on_s3 = "e47293c0bd7f39540245913f7f75be3d6863183c"; + let mainnet = Network::mainnet_unchecked().unwrap(); + let governance_version = governance_canister_version(&mainnet.nns_urls).await.unwrap(); + + let tests = &[ + TestScenario::new("match governance canister") + .delete_status_file() + .should_contain(&governance_version.stringified_hash), + TestScenario::new("use default version") + .delete_status_file() + .version(IcAdminVersion::Default) + .should_contain(DEFAULT_IC_ADMIN_VERSION), + TestScenario::new("existing version on s3") + .delete_status_file() + .version(IcAdminVersion::Strict(version_on_s3.to_string())) + .should_contain(version_on_s3), + TestScenario::new("random version not present on s3").version(IcAdminVersion::Strict("random-version".to_string())), + ]; + + for test in tests { + let mut deleted_status_file: PathBuf = dirs::home_dir().unwrap(); + if test.should_delete_status_file { + deleted_status_file = get_deleted_status_file(); + } + + let maybe_ctx = get_context(&mainnet, test.version.clone()).await; + + if let Some(ver) = test.should_contain { + assert!( + maybe_ctx.is_ok(), + "Test `{}`: expected to create DreContext, but got error: {:?}", + test.name, + maybe_ctx.err().unwrap() + ); + let ctx = maybe_ctx.unwrap(); + + let ic_admin_path = ctx.ic_admin_path().unwrap(); + assert!( + ic_admin_path.contains(ver), + "Test `{}`: ic_admin_path `{}`, expected version `{}`", + test.name, + ic_admin_path, + ver + ) + } else { + assert!( + maybe_ctx.is_err(), + "Test `{}`: expected error but got ok with version: {}", + test.name, + maybe_ctx.unwrap().ic_admin_path().unwrap() + ); + } + + if test.should_delete_status_file { + assert!( + deleted_status_file.exists(), + "Test `{}`: expected ic-admin.status file to be recreated, but it wasn't", + test.name + ) + } + } +} diff --git a/rs/cli/src/unit_tests/mod.rs b/rs/cli/src/unit_tests/mod.rs index c8e00fed4..acccbf331 100644 --- a/rs/cli/src/unit_tests/mod.rs +++ b/rs/cli/src/unit_tests/mod.rs @@ -1 +1,2 @@ +mod ctx_init; mod update_unassigned_nodes; diff --git a/rs/qualifier/src/qualify_util.rs b/rs/qualifier/src/qualify_util.rs index a03258f15..9b2f18020 100644 --- a/rs/qualifier/src/qualify_util.rs +++ b/rs/qualifier/src/qualify_util.rs @@ -85,6 +85,7 @@ pub async fn qualify( false, cmd.require_ic_admin(), None, + dre::commands::IcAdminVersion::FromGovernance, ) .await?;