ParthibanRajasekaran commented on code in PR #4350: URL: https://github.com/apache/iggy/pull/4350#discussion_r4198687557
########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,294 @@ +// 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 assert_cmd::Command; +use std::collections::HashSet; +use std::time::Duration; + +const LIST_ENV_VARS_TIMEOUT: Duration = Duration::from_secs(5); + +fn list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> Vec<String> +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let mut cmd = Command::cargo_bin(binary).expect("binary should be built"); + cmd.current_dir(directory) + .arg("--list-config-env-vars") + .envs(env) + .args(extra_args) + .timeout(LIST_ENV_VARS_TIMEOUT); + + let output = cmd.output().expect("listing command should run"); + assert!( + output.status.success(), + "{binary} failed: {:?}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stderr) + ); + assert!(output.stderr.is_empty(), "{binary} wrote to stderr"); + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names = stdout.lines().map(str::to_owned).collect::<Vec<_>>(); + assert!(!names.is_empty(), "{binary} returned no variables"); + names +} + +fn list_config_env_vars(binary: &str) -> Vec<String> { + let directory = tempfile::tempdir().expect("temporary directory"); + list_config_env_vars_in( + binary, + directory.path(), + std::iter::empty::<(&str, &str)>(), + &[], + ) +} + +#[test] +fn config_env_listing_exits_before_startup_for_each_binary() { + for (binary, config_env, dotenv_env, invalid_env) in [ + ( + "iggy-server", + "IGGY_CONFIG_PATH", + "IGGY_ENV_PATH", + "IGGY_HTTP_ADDRESS", + ), + ( + "iggy-connectors", + "IGGY_CONNECTORS_CONFIG_PATH", + "IGGY_CONNECTORS_ENV_PATH", + "IGGY_CONNECTORS_HTTP_ADDRESS", + ), + ( + "iggy-mcp", + "IGGY_MCP_CONFIG_PATH", + "IGGY_MCP_ENV_PATH", + "IGGY_MCP_HTTP_ADDRESS", + ), + ] { + let directory = tempfile::tempdir().expect("temporary directory"); + let config_path = directory.path().join("invalid-config.toml"); + let dotenv_path = directory.path().join("invalid.env"); + std::fs::write(&config_path, "not valid toml = [").expect("write invalid config"); + std::fs::write(&dotenv_path, "not a dotenv assignment").expect("write invalid dotenv"); + + let entries_before: HashSet<_> = std::fs::read_dir(directory.path()) + .expect("read temporary directory") + .map(|entry| entry.expect("directory entry").file_name()) + .collect(); + let names = list_config_env_vars_in( + binary, + directory.path(), + [ + (config_env, config_path.into_os_string()), + (dotenv_env, dotenv_path.into_os_string()), + (invalid_env, "invalid-value".into()), + ], + &[], + ); + + let baseline = list_config_env_vars(binary); + assert_eq!( + names, baseline, + "{binary} output changed with invalid config, dotenv or env values" + ); + let entries_after: HashSet<_> = std::fs::read_dir(directory.path()) + .expect("read temporary directory") + .map(|entry| entry.expect("directory entry").file_name()) + .collect(); + assert_eq!( + entries_after, entries_before, + "{binary} created startup side effects" + ); + + assert!( + names.windows(2).all(|pair| pair[0] < pair[1]), + "{binary} output must be sorted and deduplicated" + ); + assert!( + names.iter().all(|name| name.starts_with("IGGY_")), + "{binary} output contained non-IGGY_ prefixed names" + ); + } +} + +#[test] +fn config_env_listing_includes_vector_index_templates() { + let names = list_config_env_vars("iggy-server"); + let names: HashSet<_> = names.iter().map(String::as_str).collect(); + + // Cluster nodes should have indexed templates + assert!( Review Comment: Fixed in 8eff7f8. Removed the implied outer-vector assertion and the WHAT comments, retaining the exact nested-vector assertion. ########## core/configs/src/configs_impl/typed_env_provider.rs: ########## @@ -314,7 +310,7 @@ impl<T: ConfigEnvMappings> TypedEnvProvider<T> { let debug_msg = match &context { WarningContext::MainConfig => format!( - "Unknown IGGY_ env var: '{}'.{}. Add to IGNORED_ENV_VARS if intentional, \ + "Unknown IGGY_ env var: '{}'.{}. Add to the runtime env-var lists if intentional, \ Review Comment: Fixed in 8eff7f8. The diagnostic now names MCP_RUNTIME_ENV_VARS and CONNECTORS_RUNTIME_ENV_VARS for advertised variables and explicitly directs internal variables to a dedicated scan-only list. ########## core/connectors/runtime/src/main.rs: ########## @@ -117,13 +141,53 @@ fn print_ascii_art(text: &str) { println!("{}", figure.unwrap()); } -#[tokio::main] -async fn main() -> Result<(), RuntimeError> { +fn main() -> Result<(), RuntimeError> { + let args = Args::parse(); + if args.list_config_env_vars { + print_config_env_vars().map_err(RuntimeError::ListConfigEnvVars)?; + return Ok(()); + } capture_allowed_cpus(); - Args::parse(); + Builder::new_multi_thread() + .enable_all() + .build() + .map_err(RuntimeError::RuntimeCreation)? + .block_on(run()) +} + +fn print_config_env_vars() -> std::io::Result<()> { + let sink_source_templates = [ + ("SINK", SinkConfig::env_templates()), + ("SOURCE", SourceConfig::env_templates()), + ] + .into_iter() + .flat_map(|(kind, templates)| { + // "<KEY>" stands in for the real, per-connector key `local_provider` + // uppercases at runtime - same prefix rule, so the listing can't + // drift from the names the runtime actually reads. + let prefix = + crate::configs::connectors::local_provider::connector_env_prefix(kind, "<KEY>"); + let plugin_config = format!("{prefix}{PLUGIN_CONFIG_ENV_SEGMENT}<FIELD>"); + templates + .iter() + .map(move |template| format!("{prefix}{}", template.env_name)) Review Comment: Fixed in 8eff7f8. The listing filters exact KEY and VERSION templates for both sink and source while preserving legacy parsing behavior. Integration coverage pins all four names as absent, and I mutation-tested the assertion by removing the filter and observing the test fail. ########## core/ai/mcp/README.md: ########## @@ -58,10 +58,14 @@ The configuration file must use TOML. The default path is `core/ai/mcp/config.to Set `IGGY_MCP_ENV_PATH` to load a particular dotenv file. Otherwise `.env` is searched for in the current directory and its parents. Existing environment variables take precedence over dotenv values. +Run `iggy-mcp --list-config-env-vars` to print the supported configuration environment variables and exit before loading dotenv or configuration files or creating the runtime. + A non-empty `iggy.token` takes precedence over username and password. It accepts a literal PAT or a `file:` reference such as `file:/run/secrets/iggy_pat`; file contents are trimmed and a leading `~/` expands to the home directory. Set `command` to the absolute path of the built executable. This Claude Desktop example uses the development broker credentials: +`iggy-mcp` rejects unknown command-line arguments with exit code 2. Keep the client or container `args` list empty unless it contains a supported flag. Review Comment: Fixed in 8eff7f8. The unknown-argument note now sits before the Claude example introduction, leaving the introduction adjacent to its JSON block. Integration coverage asserts exit code 2, empty stdout, and the unexpected-argument diagnostic. ########## core/connectors/runtime/src/main.rs: ########## @@ -117,13 +141,53 @@ fn print_ascii_art(text: &str) { println!("{}", figure.unwrap()); } -#[tokio::main] -async fn main() -> Result<(), RuntimeError> { +fn main() -> Result<(), RuntimeError> { + let args = Args::parse(); + if args.list_config_env_vars { + print_config_env_vars().map_err(RuntimeError::ListConfigEnvVars)?; + return Ok(()); + } capture_allowed_cpus(); - Args::parse(); + Builder::new_multi_thread() + .enable_all() + .build() + .map_err(RuntimeError::RuntimeCreation)? + .block_on(run()) +} + +fn print_config_env_vars() -> std::io::Result<()> { + let sink_source_templates = [ + ("SINK", SinkConfig::env_templates()), Review Comment: Fixed in 8eff7f8. The listing imports connector_env_prefix at module scope and derives sink/source labels through BaseConnectorConfig::connector_type instead of duplicating uppercase literals. ########## core/configs/src/configs_impl/env_mapping.rs: ########## @@ -59,3 +78,41 @@ pub trait ConfigEnvMappings { .collect() } } + +impl EnvVarTemplate { + fn expand(&self) -> Vec<EnvVarMapping> { Review Comment: Fixed in 8eff7f8. Expansion now debug-asserts that both env-name and config-path placeholder counts match max_elements. Focused tests cover valid two-level expansion and each malformed dimension. ########## core/server/src/args.rs: ########## @@ -87,6 +87,25 @@ For more information, visit: https://iggy.apache.org/docs/introduction/getting-s // variable names and paths must stay unquoted rather than wear rustdoc backticks. #[allow(clippy::doc_markdown)] pub struct Args { + #[arg( + long, + help = "Print supported configuration environment variables and exit", + long_help = r#"Print supported configuration environment variables and exit. + +Lists all supported IGGY_* environment variable names and templates, +sorted and deduplicated. Template syntax: +- <N> represents vector indices (0-255 for most fields, except Review Comment: Fixed in 8eff7f8. Server and connector help tests now pin the displayed inclusive ranges to representative env_templates max_elements values. The server help no longer mentions plugins. ########## core/configs/src/server_config/server.rs: ########## @@ -47,23 +47,42 @@ pub use crate::common::server::{ TelemetryConfig, TelemetryLogsConfig, TelemetryTracesConfig, TelemetryTransport, }; -pub const SERVER_PROCESS_ENV_VARS: &[&str] = &[ - "IGGY_CONFIG_PATH", - "IGGY_ENV_PATH", - "IGGY_DISPLAY_CONFIG", - "IGGY_ROOT_USERNAME", - "IGGY_ROOT_PASSWORD", +/// Vars used by sibling binaries (iggy CLI) or test/CI-only. These suppress +/// unknown-name warnings but are not advertised by the server. +const SERVER_SCAN_ONLY_ENV_VARS: &[&str] = &[ "IGGY_TEST_VERBOSE", "IGGY_TEST_CLUSTER_NODES", "IGGY_TEST_CLEANUP_DISABLED", - "IGGY_SHARD_RUNTIME_CAPACITY", - "IGGY_SHARD_EVENT_INTERVAL", "IGGY_CI_BUILD", "IGGY_HOME", "IGGY_USERNAME", "IGGY_PASSWORD", ]; +/// Non-config env vars supported by the server and advertised to operators. +const SERVER_RUNTIME_ENV_VARS: &[&str] = &[ + "IGGY_CONFIG_PATH", + "IGGY_ENV_PATH", + "IGGY_DISPLAY_CONFIG", Review Comment: Fixed in 8eff7f8. DISPLAY_CONFIG_ENV now has an internal pub(crate) re-export and the server runtime list reuses it without expanding the public API. ########## core/configs/src/configs_impl/env_listing.rs: ########## @@ -0,0 +1,102 @@ +// 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. + +/// Writes each name in `names`, sorted and deduplicated, to the provided writer — one Review Comment: Fixed in 8eff7f8. Replaced the em dash with normal sentence punctuation. ########## core/configs/src/configs_impl/env_mapping.rs: ########## @@ -59,3 +78,41 @@ pub trait ConfigEnvMappings { .collect() } } + +impl EnvVarTemplate { + fn expand(&self) -> Vec<EnvVarMapping> { + let mut results = vec![(self.env_name.to_string(), self.config_path.to_string())]; + + for &limit in self.max_elements { + let mut next = Vec::new(); + for (env_name, config_path) in results { + for i in 0..limit { + let index = i.to_string(); + next.push(( + env_name.replacen("<N>", &index, 1), + config_path.replacen("<N>", &index, 1), + )); + } + } + results = next; + } + + results + .into_iter() + .map(|(env_name, config_path)| EnvVarMapping { + env_name: Box::leak(env_name.into_boxed_str()), + config_path: Box::leak(config_path.into_boxed_str()), + is_secret: self.is_secret, + }) + .collect() + } +} + +/// Expands compact templates into the mappings consumed by the env provider. +/// +/// The returned mappings own leaked names and paths so generated +/// `ConfigEnvMappings` implementations can cache them in a `OnceLock` and +/// return `'static` references. Call this once per config type. +pub fn expand_env_templates(templates: &[EnvVarTemplate]) -> Vec<EnvVarMapping> { Review Comment: Fixed in 8eff7f8. expand_env_templates remains public for generated downstream code but is now doc-hidden, with its intentional leak and OnceLock usage contract retained. -- 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]
