hubcio commented on code in PR #2998: URL: https://github.com/apache/iggy/pull/2998#discussion_r2994228734
########## core/cli/src/commands/binary_context/delete_context.rs: ########## @@ -0,0 +1,61 @@ +/* Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +use async_trait::async_trait; +use tracing::{Level, event}; + +use crate::commands::cli_command::{CliCommand, PRINT_TARGET}; +use iggy_common::Client; + +use super::common::ContextManager; + +pub struct DeleteContextCmd { + context_name: String, +} + +impl DeleteContextCmd { + pub fn new(context_name: String) -> Self { + Self { context_name } + } +} + +#[async_trait] +impl CliCommand for DeleteContextCmd { + fn explain(&self) -> String { + let context_name = &self.context_name; + format!("delete context {context_name}") + } + + fn login_required(&self) -> bool { + false + } + + fn connection_required(&self) -> bool { + false + } + + async fn execute_cmd(&mut self, _client: &dyn Client) -> anyhow::Result<(), anyhow::Error> { + let mut context_mgr = ContextManager::default(); + + context_mgr.delete_context(&self.context_name).await?; + + event!(target: PRINT_TARGET, Level::INFO, "context '{}' deleted successfully", self.context_name); + + Ok(()) + } +} Review Comment: nit: `create_context.rs` has a `#[cfg(test)]` module with tests for `explain()`, `login_required()`, and `connection_required()`. `delete_context.rs` is missing the same for consistency. ########## core/cli/src/commands/binary_context/common.rs: ########## @@ -126,6 +126,69 @@ impl ContextManager { Ok(context_state.contexts.clone()) } + pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> { Review Comment: no validation on `name` - empty strings, whitespace-only, and control characters are all accepted. an empty string creates a `[""] ` TOML section. the toml crate handles quoting for most special chars and fails cleanly for truly bad ones (e.g. newlines in table keys), so this isn't a corruption risk, but it produces confusing config entries. a basic check like `name.trim().is_empty()` plus maybe an alphanumeric+hyphen+underscore filter would be a good guard. also: `create_context("default", ...)` is implicitly blocked because `get_context_state` always seeds a "default" entry, so `contains_key` catches it. but an explicit `if name == DEFAULT_CONTEXT_NAME { bail!(...) }` guard (like `delete_context` has) would be more resilient to future changes. ########## core/cli/src/args/context.rs: ########## @@ -50,3 +75,134 @@ pub(crate) struct ContextUseArgs { #[arg(value_parser = clap::value_parser!(String))] pub(crate) context_name: String, } + +#[derive(Debug, Clone, Args)] +pub(crate) struct ContextCreateArgs { + /// Name of the context to create + #[arg(value_parser = clap::value_parser!(String))] + pub(crate) context_name: String, + + /// The transport to use + /// + /// Valid values are "quic", "http", "tcp" and "ws". + #[clap(verbatim_doc_comment, long)] + pub(crate) transport: Option<String>, Review Comment: `transport` is a free-form `Option<String>` with no parse-time validation. `--transport foobar` gets silently written to `contexts.toml` and only fails later at connection time with an unrelated error. a `TransportProtocol` enum already exists in the codebase (`core/common/src/types/configuration/transport.rs`) with strum-based `FromStr`. you could validate here with `clap::value_parser!` or at minimum call `TransportProtocol::from_str` in `create_context` before writing. this follows the pre-existing `ArgsOptional` pattern, so not a regression - but `context create` is a new user-facing entry point where fail-fast validation matters more. also heads up: there's a pre-existing mismatch between `TransportProtocol`'s strum serialization (`"ws"`) and `Args::get_server_address` which matches against `"websocket"`. a user creating a context with `--transport ws` would hit this. ########## core/cli/src/commands/binary_context/common.rs: ########## @@ -258,6 +321,18 @@ impl ContextReaderWriter { Ok(()) } + pub fn ensure_iggy_home_exists(&self) -> Result<()> { + if let Some(ref iggy_home) = self.iggy_home + && !iggy_home.exists() + { + std::fs::create_dir_all(iggy_home).context(format!( Review Comment: `std::fs::create_dir_all` is a blocking call inside an async context. also `iggy_home.exists()` in the guard is blocking. `tokio::fs::create_dir_all` would be the async-consistent replacement. not a practical issue for a CLI, but inconsistent with the rest of this file which uses `tokio::fs` everywhere. ########## core/cli/src/commands/binary_context/common.rs: ########## @@ -126,6 +126,69 @@ impl ContextManager { Ok(context_state.contexts.clone()) } + pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> { + self.get_context_state().await?; + let cs = self.context_state.as_ref().unwrap(); + + if cs.contexts.contains_key(name) { + bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}") + } + + let mut new_contexts = cs.contexts.clone(); + new_contexts.insert(name.to_string(), config); + + self.context_rw.ensure_iggy_home_exists()?; + self.context_rw + .write_contexts(new_contexts.clone()) Review Comment: this is the first production code path that writes credentials (`--password`, `--token`) to `contexts.toml`. `tokio::fs::write` creates files with default permissions (typically `0644` - world-readable). should set `0600` on the file after writing, or use a file-creation API that sets restrictive permissions. same applies to the `write_contexts` call in `delete_context` at line 180. ########## core/cli/src/commands/binary_context/common.rs: ########## Review Comment: `#[serde(flatten)]` on `ArgsOptional` means unknown fields are silently dropped during deserialization. since `create_context` and `delete_context` do a read-all/modify-one/write-all cycle on `contexts.toml`, running an older CLI binary against a config written by a newer version will strip any new fields from ALL context entries - not just the one being created/deleted. consider adding `#[serde(flatten)] pub extra: HashMap<String, toml::Value>` to `ContextConfig` to preserve unknown keys through round-trips. ########## core/cli/src/commands/binary_context/common.rs: ########## @@ -126,6 +126,69 @@ impl ContextManager { Ok(context_state.contexts.clone()) } + pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> { + self.get_context_state().await?; + let cs = self.context_state.as_ref().unwrap(); + + if cs.contexts.contains_key(name) { + bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}") + } + + let mut new_contexts = cs.contexts.clone(); + new_contexts.insert(name.to_string(), config); + + self.context_rw.ensure_iggy_home_exists()?; + self.context_rw + .write_contexts(new_contexts.clone()) + .await + .context(format!("failed writing contexts after creating '{name}'"))?; + + self.context_state.replace(ContextState { + active_context: cs.active_context.clone(), + contexts: new_contexts, + }); + + Ok(()) + } + + pub async fn delete_context(&mut self, name: &str) -> Result<()> { + if name == DEFAULT_CONTEXT_NAME { + bail!("cannot delete the '{DEFAULT_CONTEXT_NAME}' context") + } + + self.get_context_state().await?; + let cs = self.context_state.take().unwrap(); Review Comment: minor: `delete_context` uses `.take().unwrap()` here while `create_context` uses `.as_ref().unwrap()` at line 131. the `.take()` pattern means `context_state` is `None` if any subsequent operation bails (lines 163, 171, 180). `create_context`'s `.as_ref()` approach is safer since it doesn't consume the state. in practice this doesn't matter - every `execute_cmd` creates a fresh `ContextManager` and discards it, so no caller ever makes a second call on the same instance. but for API consistency and robustness, matching `create_context`'s pattern would be cleaner. (the pre-existing `set_active_context_key` at line 100 has the same `.take()` pattern.) ########## core/integration/tests/cli/context/common.rs: ########## @@ -64,6 +64,10 @@ impl TestIggyContext { self.context_manager.read_active_context().await.unwrap() Review Comment: `read_active_context` returns the raw `read_to_string` result without `.trim()`. if a user (or text editor) adds a trailing newline to `.active_context`, the key won't match any context and `get_context_state` bails with a confusing "missing" error. adding `.map(|s| s.trim().to_string())` is cheap insurance. ########## core/cli/src/commands/binary_context/common.rs: ########## Review Comment: `ContextsConfigMap` is `HashMap<String, ContextConfig>`. every write may reorder sections in `contexts.toml`. `BTreeMap` would give stable, alphabetically sorted output. pre-existing, but worth a follow-up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
