From 592799df9a64386a3303a58a0cd9713b71c0d139 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Tue, 18 Nov 2025 16:02:55 -0800 Subject: [PATCH 1/3] Replace assert_env_contains_canister with a getter --- crates/icp-cli/src/commands/build.rs | 27 +++++++----------- crates/icp-cli/src/commands/deploy/mod.rs | 24 ++++++++-------- crates/icp-cli/src/commands/sync.rs | 34 ++++++++++------------- crates/icp/src/context/mod.rs | 18 ++++++------ crates/icp/src/lib.rs | 4 +-- 5 files changed, 47 insertions(+), 60 deletions(-) diff --git a/crates/icp-cli/src/commands/build.rs b/crates/icp-cli/src/commands/build.rs index 8b7261bf2..bfbfc5f15 100644 --- a/crates/icp-cli/src/commands/build.rs +++ b/crates/icp-cli/src/commands/build.rs @@ -1,5 +1,5 @@ -use anyhow::anyhow; use clap::Args; +use futures::future::try_join_all; use icp::context::{Context, EnvironmentSelection, GetEnvironmentError}; use crate::{ @@ -24,6 +24,9 @@ pub(crate) enum CommandError { #[error(transparent)] GetEnvironment(#[from] GetEnvironmentError), + #[error(transparent)] + GetEnvCanister(#[from] icp::context::GetEnvCanisterError), + #[error(transparent)] Project(#[from] icp::LoadError), @@ -35,9 +38,6 @@ pub(crate) async fn exec(ctx: &Context, args: &BuildArgs) -> Result<(), CommandE // Get environment selection let environment_selection: EnvironmentSelection = args.environment.clone().into(); - // Load the project manifest - let p = ctx.project.load().await?; - // Load target environment let env = ctx.get_environment(&environment_selection).await?; @@ -50,26 +50,19 @@ pub(crate) async fn exec(ctx: &Context, args: &BuildArgs) -> Result<(), CommandE false => args.canisters.clone(), }; - // Validate all specified canisters exist in project and environment - for name in &cnames { - ctx.assert_env_contains_canister(name, &environment_selection) - .await - .map_err(|e| anyhow!(e))?; - } - // Skip doing any work if no canisters are targeted if cnames.is_empty() { return Ok(()); } + let canisters_to_build = try_join_all( + cnames + .iter() + .map(|name| ctx.get_canister_and_path_for_env(name, &environment_selection)), + ) + .await?; // Build the selected canisters let _ = ctx.term.write_line("Building canisters:"); - let canisters_to_build = p - .canisters - .iter() - .filter(|(k, _)| cnames.contains(k)) - .map(|(_, (path, canister))| (path.clone(), canister.clone())) - .collect::>(); build_many_with_progress_bar( canisters_to_build, diff --git a/crates/icp-cli/src/commands/deploy/mod.rs b/crates/icp-cli/src/commands/deploy/mod.rs index 281c55461..50c0fad5c 100644 --- a/crates/icp-cli/src/commands/deploy/mod.rs +++ b/crates/icp-cli/src/commands/deploy/mod.rs @@ -3,7 +3,7 @@ use clap::Args; use futures::{StreamExt, future::try_join_all, stream::FuturesOrdered}; use ic_agent::export::Principal; use icp::{ - context::{Context, EnvironmentSelection}, + context::{Context, EnvironmentSelection, GetEnvCanisterError}, identity::IdentitySelection, }; use std::sync::Arc; @@ -57,6 +57,9 @@ pub(crate) enum CommandError { #[error("project does not contain an environment named '{name}'")] EnvironmentNotFound { name: String }, + #[error(transparent)] + GetEnvCanister(#[from] GetEnvCanisterError), + #[error(transparent)] Create(#[from] create::CommandError), @@ -93,25 +96,20 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), Command false => args.names.clone(), }; - for name in &cnames { - ctx.assert_env_contains_canister(name, &environment_selection) - .await - .map_err(|e| anyhow!(e))?; - } - // Skip doing any work if no canisters are targeted if cnames.is_empty() { return Ok(()); } + let canisters_to_build = try_join_all( + cnames + .iter() + .map(|name| ctx.get_canister_and_path_for_env(name, &environment_selection)), + ) + .await?; + // Build the selected canisters let _ = ctx.term.write_line("Building canisters:"); - let canisters_to_build = p - .canisters - .iter() - .filter(|(k, _)| cnames.contains(k)) - .map(|(_, (path, canister))| (path.clone(), canister.clone())) - .collect::>(); build_many_with_progress_bar( canisters_to_build, diff --git a/crates/icp-cli/src/commands/sync.rs b/crates/icp-cli/src/commands/sync.rs index 0fb2fcc35..1b626ec2f 100644 --- a/crates/icp-cli/src/commands/sync.rs +++ b/crates/icp-cli/src/commands/sync.rs @@ -30,6 +30,12 @@ pub(crate) enum CommandError { #[error(transparent)] Project(#[from] icp::LoadError), + #[error(transparent)] + GetEnvCanister(#[from] icp::context::GetEnvCanisterError), + + #[error(transparent)] + GetEnvCanisterId(#[from] icp::context::GetCanisterIdForEnvError), + #[error(transparent)] Unexpected(#[from] anyhow::Error), } @@ -54,13 +60,6 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), CommandEr false => args.canisters.clone(), }; - // Validate all specified canisters exist in project and environment - for name in &cnames { - ctx.assert_env_contains_canister(name, &environment_selection) - .await - .map_err(|e| anyhow!(e))?; - } - // Skip doing any work if no canisters are targeted if cnames.is_empty() { return Ok(()); @@ -73,19 +72,14 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), CommandEr .map_err(|e| anyhow!(e))?; // Prepare list of canisters with their info for syncing - let env_canisters = &env.canisters; - let sync_canisters = try_join_all(cnames.iter().map(|name| { - let environment_selection = environment_selection.clone(); - async move { - let cid = ctx - .get_canister_id_for_env(name, &environment_selection) - .await - .map_err(|e| anyhow!(e))?; - let (canister_path, info) = env_canisters - .get(name) - .ok_or_else(|| anyhow!("Canister id exists but no canister info"))?; - Ok::<_, anyhow::Error>((cid, canister_path.clone(), info.clone())) - } + let sync_canisters = try_join_all(cnames.iter().map(|name| async { + let cid = ctx + .get_canister_id_for_env(name, &environment_selection) + .await?; + let (canister_path, info) = ctx + .get_canister_and_path_for_env(name, &environment_selection) + .await?; + Ok::<_, anyhow::Error>((cid, canister_path.clone(), info.clone())) })) .await?; diff --git a/crates/icp/src/context/mod.rs b/crates/icp/src/context/mod.rs index d12f7f2c2..45dd9e8b4 100644 --- a/crates/icp/src/context/mod.rs +++ b/crates/icp/src/context/mod.rs @@ -3,11 +3,13 @@ use std::sync::Arc; use url::Url; use crate::{ + Canister, agent::CreateAgentError, canister::{build::Build, sync::Synchronize}, directories, identity::IdentitySelection, network::{Configuration as NetworkConfiguration, access::NetworkAccess}, + prelude::*, project::DEFAULT_LOCAL_ENVIRONMENT_NAME, store_id::IdMapping, }; @@ -152,26 +154,26 @@ impl Context { } } - pub async fn assert_env_contains_canister( + pub async fn get_canister_and_path_for_env( &self, canister_name: &str, environment: &EnvironmentSelection, - ) -> Result<(), AssertEnvContainsCanisterError> { + ) -> Result<(PathBuf, Canister), GetEnvCanisterError> { let p = self.project.load().await?; - if !p.contains_canister(canister_name) { - return Err(AssertEnvContainsCanisterError::CanisterNotFoundInProject { + let Some((path, canister)) = p.get_canister(canister_name) else { + return Err(GetEnvCanisterError::CanisterNotFoundInProject { canister_name: canister_name.to_owned(), }); - } + }; let env = self.get_environment(environment).await?; if !env.contains_canister(canister_name) { - return Err(AssertEnvContainsCanisterError::CanisterNotInEnv { + return Err(GetEnvCanisterError::CanisterNotInEnv { canister_name: canister_name.to_owned(), environment_name: environment.name().to_owned(), }); } - Ok(()) + Ok((path.clone(), canister.clone())) } /// Gets the canister ID for a given canister name in a specified environment. @@ -599,7 +601,7 @@ pub enum GetIdsByEnvironmentError { } #[derive(Debug, Snafu)] -pub enum AssertEnvContainsCanisterError { +pub enum GetEnvCanisterError { #[snafu(transparent)] ProjectLoad { source: crate::LoadError }, diff --git a/crates/icp/src/lib.rs b/crates/icp/src/lib.rs index 0ac11ad87..5163058cd 100644 --- a/crates/icp/src/lib.rs +++ b/crates/icp/src/lib.rs @@ -93,8 +93,8 @@ pub struct Project { } impl Project { - pub fn contains_canister(&self, canister_name: &str) -> bool { - self.canisters.contains_key(canister_name) + pub fn get_canister(&self, canister_name: &str) -> Option<&(PathBuf, Canister)> { + self.canisters.get(canister_name) } } From fb3558c30a8ba53fcb8c0de4750e5f41992ad91d Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Tue, 18 Nov 2025 17:15:55 -0800 Subject: [PATCH 2/3] fix test --- crates/icp-cli/tests/sync_tests.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/icp-cli/tests/sync_tests.rs b/crates/icp-cli/tests/sync_tests.rs index 34d6730a8..a86fe6b28 100644 --- a/crates/icp-cli/tests/sync_tests.rs +++ b/crates/icp-cli/tests/sync_tests.rs @@ -261,20 +261,28 @@ fn sync_with_valid_principal() { steps: - type: script command: echo syncing + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} "#}; write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + // Start network + let _g = ctx.start_network_in(&project_dir, "my-network"); + ctx.ping_until_healthy(&project_dir, "my-network"); + // Valid principal let principal = "aaaaa-aa"; // Try to sync with principal (should fail) ctx.icp() .current_dir(&project_dir) - .args(["sync", principal]) + .args(["sync", principal, "--environment", "my-environment"]) .assert() .failure() - .stderr(contains("project does not contain a canister named")); + .stderr(contains( + "canister 'aaaaa-aa' not found in environment 'my-environment'", + )); } #[test] From 34e7b96f81b57031422e6b16e37f9611773decdf Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Tue, 18 Nov 2025 17:20:37 -0800 Subject: [PATCH 3/3] unfix test --- crates/icp-cli/src/commands/sync.rs | 6 +++--- crates/icp-cli/tests/sync_tests.rs | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/icp-cli/src/commands/sync.rs b/crates/icp-cli/src/commands/sync.rs index 1b626ec2f..a84c5e7ef 100644 --- a/crates/icp-cli/src/commands/sync.rs +++ b/crates/icp-cli/src/commands/sync.rs @@ -73,12 +73,12 @@ pub(crate) async fn exec(ctx: &Context, args: &SyncArgs) -> Result<(), CommandEr // Prepare list of canisters with their info for syncing let sync_canisters = try_join_all(cnames.iter().map(|name| async { - let cid = ctx - .get_canister_id_for_env(name, &environment_selection) - .await?; let (canister_path, info) = ctx .get_canister_and_path_for_env(name, &environment_selection) .await?; + let cid = ctx + .get_canister_id_for_env(name, &environment_selection) + .await?; Ok::<_, anyhow::Error>((cid, canister_path.clone(), info.clone())) })) .await?; diff --git a/crates/icp-cli/tests/sync_tests.rs b/crates/icp-cli/tests/sync_tests.rs index a86fe6b28..358bcd7a0 100644 --- a/crates/icp-cli/tests/sync_tests.rs +++ b/crates/icp-cli/tests/sync_tests.rs @@ -280,9 +280,7 @@ fn sync_with_valid_principal() { .args(["sync", principal, "--environment", "my-environment"]) .assert() .failure() - .stderr(contains( - "canister 'aaaaa-aa' not found in environment 'my-environment'", - )); + .stderr(contains("project does not contain a canister named")); } #[test]