hubcio commented on code in PR #4350: URL: https://github.com/apache/iggy/pull/4350#discussion_r4174556018
########## core/configs/src/configs_impl/env_listing.rs: ########## @@ -0,0 +1,59 @@ +// 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 +/// call, one error policy. A closed writer (e.g. `iggy-server --list-config-env-vars | head -1`) +/// is expected, not a failure: on `BrokenPipe` this stops writing and returns `Ok(())`. Any +/// other I/O error is real and is propagated so the caller exits non-zero. +pub fn print_env_var_names<I, S, W>(names: I, writer: &mut W) -> std::io::Result<()> Review Comment: nit: the writer param is in now, but nothing drives it - add two unit tests with writers that fail with `BrokenPipe` (expect `Ok`) and `StorageFull` (expect `Err`). ########## core/configs/src/configs_impl/typed_env_provider.rs: ########## @@ -625,19 +620,13 @@ mod tests { #[test] Review Comment: nit: the doc comment above still says the list check holds in both build profiles, but the `assert!` loop is gone and a release build now asserts nothing. restore the check or drop that sentence. ########## core/configs/src/configs_impl/typed_env_provider.rs: ########## @@ -625,19 +620,13 @@ mod tests { #[test] #[serial_test::serial] fn ignored_env_vars_are_skipped_by_the_unknown_variable_scan() { - for name in [ - "IGGY_CONNECTORS_CONFIG_PATH", - "IGGY_CONNECTORS_ENV_PATH", - "IGGY_MCP_CONFIG_PATH", - "IGGY_MCP_ENV_PATH", - ] { - assert!( - IGNORED_ENV_VARS.contains(&name), - "{name} is read by a sibling binary before its config loads, so the scan must skip it" - ); - } + let runtime_env_vars = super::super::CONNECTORS_RUNTIME_ENV_VARS Review Comment: warning: this test now takes its names from the same lists `is_runtime_env_var` reads, so it still passes if a path name drops out, and a debug build then panics when that var is set. pin the four literal names again. ########## core/connectors/runtime/src/main.rs: ########## @@ -117,13 +139,52 @@ 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()?; + return Ok(()); + } capture_allowed_cpus(); - Args::parse(); + Builder::new_multi_thread() + .enable_all() + .build()? + .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_<FIELD>"); Review Comment: nit: `connector_env_prefix` covers the prefix now, but `PLUGIN_CONFIG_` is still typed here, at `local_provider.rs:352` and at `typed_env_provider.rs:295`. one constant in `configs` can serve all three. ########## core/configs/src/configs_impl/env_mapping.rs: ########## @@ -59,3 +78,38 @@ pub trait ConfigEnvMappings { .collect() } } + +impl EnvVarTemplate { + /// Expands this template into every concrete mapping it represents. + pub fn expand(&self) -> Vec<EnvVarMapping> { Review Comment: nit: `expand` has one caller (`expand_env_templates`) and leaks two strings per mapping on every call - make it private and note the leak on `expand_env_templates`. ########## core/connectors/runtime/src/configs/connectors/local_provider.rs: ########## @@ -395,6 +398,20 @@ impl BaseConnectorConfig { } } +/// Builds the env-var prefix a connector's config and plugin-config overrides +/// are matched against: `IGGY_CONNECTORS_{TYPE}_{KEY}_`. `connector_type` and +/// `key` are expected uppercased, as `BaseConnectorConfig`'s callers already +/// do. Shared by the runtime's actual override lookup here and by +/// `--list-config-env-vars`'s listing in `main.rs` (passing the literal +/// `<KEY>` as the key), so the two can't drift apart on the prefix the +/// server actually reads. Review Comment: nit: s/server/connectors runtime/ - only the connectors runtime reads this prefix, the server just allow-lists `IGGY_CONNECTORS_` for its unknown-var scan. ########## core/configs/src/configs_impl/env_listing.rs: ########## @@ -0,0 +1,59 @@ +// 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 +/// call, one error policy. A closed writer (e.g. `iggy-server --list-config-env-vars | head -1`) +/// is expected, not a failure: on `BrokenPipe` this stops writing and returns `Ok(())`. Any +/// other I/O error is real and is propagated so the caller exits non-zero. +pub fn print_env_var_names<I, S, W>(names: I, writer: &mut W) -> std::io::Result<()> +where + I: IntoIterator<Item = S>, + S: Into<String>, + W: std::io::Write, +{ + let mut names: Vec<String> = names.into_iter().map(Into::into).collect(); + names.sort_unstable(); + names.dedup(); + + for name in names { + if let Err(err) = writeln!(writer, "{name}") { + return if err.kind() == std::io::ErrorKind::BrokenPipe { + Ok(()) + } else { + Err(err) + }; + } + } + Ok(()) +} + +/// Env vars `iggy-mcp --list-config-env-vars` advertises beyond the derived +/// `McpServerConfig` templates. +pub const MCP_CONFIG_PATH_ENV: &str = "IGGY_MCP_CONFIG_PATH"; +pub const MCP_ENV_PATH_ENV: &str = "IGGY_MCP_ENV_PATH"; +pub const MCP_RUNTIME_ENV_VARS: &[&str] = + &["IGGY_DISPLAY_CONFIG", MCP_CONFIG_PATH_ENV, MCP_ENV_PATH_ENV]; Review Comment: nit: `"IGGY_DISPLAY_CONFIG"` is still a literal here and at line 58, but `file_provider.rs:29` already has `DISPLAY_CONFIG_ENV`. make it `pub(crate)` and use it in both lists. ########## core/connectors/runtime/src/main.rs: ########## @@ -117,13 +139,52 @@ 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()?; Review Comment: nit: server and MCP got `ListConfigEnvVars`, but here the listing write error and the runtime build error at line 151 both still land in the generic `RuntimeError::IoError`. add the two variants like `McpRuntimeError`. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { + cmd.env(key, value); + } + + for arg in extra_args { + cmd.arg(arg); + } + + cmd.output().expect("listing command should run") +} + +/// Convenience wrapper over [`run_list_config_env_vars_in`] for tests that +/// don't need to inspect the working directory afterward. +fn run_list_config_env_vars<I, K, V>( + binary: &str, + env: I, + extra_args: &[&str], +) -> std::process::Output +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let directory = tempfile::tempdir().expect("temporary directory"); + run_list_config_env_vars_in(binary, directory.path(), env, extra_args) +} + +/// Convenience wrapper over [`run_list_config_env_vars`] for tests that don't +/// need to set any environment variables. +fn run_list_config_env_vars_plain(binary: &str, extra_args: &[&str]) -> std::process::Output { + run_list_config_env_vars(binary, std::iter::empty::<(&str, &str)>(), extra_args) +} + +#[test] +fn config_env_listing_exits_before_startup_for_each_binary() { Review Comment: nit: nothing asserts that each binary lists its own runtime vars like `IGGY_ROOT_PASSWORD`, `IGGY_SHARD_RUNTIME_CAPACITY` or `IGGY_MCP_CONFIG_PATH`, so dropping the `.chain(...)` in the server main keeps every test green. assert a few by name. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { + cmd.env(key, value); + } + + for arg in extra_args { + cmd.arg(arg); + } + + cmd.output().expect("listing command should run") +} + +/// Convenience wrapper over [`run_list_config_env_vars_in`] for tests that +/// don't need to inspect the working directory afterward. +fn run_list_config_env_vars<I, K, V>( + binary: &str, + env: I, + extra_args: &[&str], +) -> std::process::Output +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let directory = tempfile::tempdir().expect("temporary directory"); + run_list_config_env_vars_in(binary, directory.path(), env, extra_args) +} + +/// Convenience wrapper over [`run_list_config_env_vars`] for tests that don't +/// need to set any environment variables. +fn run_list_config_env_vars_plain(binary: &str, extra_args: &[&str]) -> std::process::Output { + run_list_config_env_vars(binary, std::iter::empty::<(&str, &str)>(), extra_args) +} + +#[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 output = run_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()), + ], + &[], + ); + + 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 baseline = run_list_config_env_vars_plain(binary, &[]); + assert_eq!( + output.stdout, baseline.stdout, + "{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" + ); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: Vec<_> = stdout.lines().collect(); + assert!(!names.is_empty(), "{binary} returned no variables"); + 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() { + // Verify that vector fields are correctly represented with <N> placeholder + let output = run_list_config_env_vars_plain("iggy-server", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Cluster nodes should have indexed templates + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_")), + "server should list cluster node index templates" + ); + + // Verify nested vector expansion (nested <N> placeholders) + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_ADVERTISED_ADDRESSES_<N>_")), + "server should list nested vector templates" + ); +} + +#[test] +fn config_env_listing_includes_connector_templates() { + // Verify that connector SINK/SOURCE templates are correctly formatted + let output = run_list_config_env_vars_plain("iggy-connectors", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Both SINK and SOURCE templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_")), + "connectors should list SINK templates with <KEY> placeholder" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_")), + "connectors should list SOURCE templates with <KEY> placeholder" + ); + + // Plugin config templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SINK plugin config templates" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SOURCE plugin config templates" + ); + assert!( + names.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SINK plugin config format" + ); + assert!( + names.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SOURCE plugin config format" + ); + + // Verify SINK and SOURCE ENABLED fields are present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_ENABLED")), + "connectors should list SINK_<KEY>_ENABLED" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_ENABLED")), + "connectors should list SOURCE_<KEY>_ENABLED" + ); + + assert_eq!( + stdout + .lines() + .filter(|name| *name == "IGGY_CONNECTORS_CONNECTORS_CONFIG_TYPE") + .count(), + 1, + "enum variants should produce one deduplicated tag name" + ); +} + +#[test] +fn config_env_listing_excludes_other_processes_variables() { + let server = run_list_config_env_vars_plain("iggy-server", &[]); + let server = String::from_utf8(server.stdout).expect("UTF-8 output"); + let server_names: HashSet<_> = server.lines().collect(); + for excluded in [ + "IGGY_CI_BUILD", + "IGGY_HOME", + "IGGY_PASSWORD", + "IGGY_TEST_CLEANUP_DISABLED", + "IGGY_TEST_CLUSTER_NODES", + "IGGY_TEST_VERBOSE", + "IGGY_USERNAME", + ] { + assert!(!server_names.contains(excluded), "server listed {excluded}"); + } + assert!( + server_names + .iter() + .all(|name| !name.starts_with("IGGY_CONNECTORS_") + && !name.starts_with("IGGY_KAFKA_") + && !name.starts_with("IGGY_MCP_")), + "server listed a sibling binary's variables" + ); + + for (binary, prefix) in [ + ("iggy-connectors", "IGGY_CONNECTORS_"), + ("iggy-mcp", "IGGY_MCP_"), + ] { + let output = run_list_config_env_vars_plain(binary, &[]); + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + assert!( + stdout + .lines() + .all(|name| name == "IGGY_DISPLAY_CONFIG" || name.starts_with(prefix)), + "{binary} listed a variable outside {prefix}" + ); + } +} + +#[test] +fn config_env_listing_exits_before_runtime_with_invalid_env_values() { Review Comment: simplification: this repeats the first test except for one empty-stderr assert without a config path. move that assert into the helper and drop the test. ########## core/configs_derive/src/config_env.rs: ########## @@ -255,108 +261,67 @@ fn generate_struct_impl( let (mappings, nested_fields) = collect_mappings(&fields); let const_definitions = generate_const_definitions(&mappings, prefix_str); - let mapping_entries = generate_mapping_entries(&mappings); let builder_methods = generate_builder_methods(&mappings, prefix_str); let builder_name = format_ident!("{}EnvBuilder", struct_name); - let mappings_count = mappings.len(); - // Generate code to include nested type mappings - let nested_extends: Vec<TokenStream2> = nested_fields - .iter() - .map(|info| { - let ty = &info.element_type; - let field_env_segment = &info.field_env_segment; - let field_name = &info.field_name; - - if info.is_vec { - // For Vec fields, expand with array indices - let max_elements = info.max_elements; - quote! { - let nested_mappings = <#ty as configs::ConfigEnvMappings>::env_mappings(); - for i in 0..#max_elements { - for mapping in nested_mappings { - // Transform env_name by prepending FIELD_INDEX_ - let new_env_name: &'static str = Box::leak( - format!("{}_{}_{}", #field_env_segment, i, mapping.env_name).into_boxed_str() - ); - // Transform config_path by prepending field.index. - let new_config_path: &'static str = Box::leak( - format!("{}.{}.{}", #field_name, i, mapping.config_path).into_boxed_str() - ); - all_mappings.push(configs::EnvVarMapping { - env_name: new_env_name, - config_path: new_config_path, - is_secret: mapping.is_secret, - }); - } - } + let own_template_entries = mappings.iter().map(|mapping| { + let env_suffix = &mapping.env_suffix; + let config_path = &mapping.config_path; + let is_secret = mapping.is_secret; + quote! { + configs::EnvVarTemplate { + env_name: #env_suffix, + config_path: #config_path, + is_secret: #is_secret, + max_elements: &[], + } + } + }); + let nested_template_extends = nested_fields.iter().map(|info| { + let ty = &info.element_type; + let segment = &info.field_env_segment; + let field_name = &info.field_name; + if info.is_vec { Review Comment: simplification: the `Vec` and plain branches emit the same loop and differ only in the `<N>` segment and the limits - pick those at macro time and emit one loop. ########## core/configs/src/server_config/server.rs: ########## @@ -47,6 +47,21 @@ pub use crate::common::server::{ TelemetryConfig, TelemetryLogsConfig, TelemetryTracesConfig, TelemetryTransport, }; +/// Vars used by sibling binaries (iggy CLI) or test/CI-only: never advertised +/// via `--list-config-env-vars`, but still non-config. Included in +/// `SERVER_PROCESS_ENV_VARS` to suppress "unknown env var" warnings during +/// config scanning. +pub const SERVER_SCAN_ONLY_ENV_VARS: &[&str] = &[ Review Comment: simplification: keep scan-only and advertised as two disjoint lists and expose the union through one `pub fn` for the scan, the tests and the harness - then this const can be private too. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { + cmd.env(key, value); + } + + for arg in extra_args { + cmd.arg(arg); + } + + cmd.output().expect("listing command should run") +} + +/// Convenience wrapper over [`run_list_config_env_vars_in`] for tests that +/// don't need to inspect the working directory afterward. +fn run_list_config_env_vars<I, K, V>( + binary: &str, + env: I, + extra_args: &[&str], +) -> std::process::Output +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let directory = tempfile::tempdir().expect("temporary directory"); + run_list_config_env_vars_in(binary, directory.path(), env, extra_args) +} + +/// Convenience wrapper over [`run_list_config_env_vars`] for tests that don't +/// need to set any environment variables. +fn run_list_config_env_vars_plain(binary: &str, extra_args: &[&str]) -> std::process::Output { + run_list_config_env_vars(binary, std::iter::empty::<(&str, &str)>(), extra_args) +} + +#[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 output = run_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()), + ], + &[], + ); + + 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 baseline = run_list_config_env_vars_plain(binary, &[]); + assert_eq!( + output.stdout, baseline.stdout, + "{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" + ); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: Vec<_> = stdout.lines().collect(); + assert!(!names.is_empty(), "{binary} returned no variables"); + 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() { + // Verify that vector fields are correctly represented with <N> placeholder + let output = run_list_config_env_vars_plain("iggy-server", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Cluster nodes should have indexed templates + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_")), + "server should list cluster node index templates" + ); + + // Verify nested vector expansion (nested <N> placeholders) + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_ADVERTISED_ADDRESSES_<N>_")), + "server should list nested vector templates" + ); +} + +#[test] +fn config_env_listing_includes_connector_templates() { + // Verify that connector SINK/SOURCE templates are correctly formatted + let output = run_list_config_env_vars_plain("iggy-connectors", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Both SINK and SOURCE templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_")), + "connectors should list SINK templates with <KEY> placeholder" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_")), + "connectors should list SOURCE templates with <KEY> placeholder" + ); + + // Plugin config templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SINK plugin config templates" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SOURCE plugin config templates" + ); + assert!( + names.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SINK plugin config format" + ); + assert!( + names.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SOURCE plugin config format" + ); + + // Verify SINK and SOURCE ENABLED fields are present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_ENABLED")), + "connectors should list SINK_<KEY>_ENABLED" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_ENABLED")), + "connectors should list SOURCE_<KEY>_ENABLED" + ); + + assert_eq!( + stdout + .lines() + .filter(|name| *name == "IGGY_CONNECTORS_CONNECTORS_CONFIG_TYPE") + .count(), + 1, + "enum variants should produce one deduplicated tag name" Review Comment: nit: no input has duplicates, so this passes with `names.dedup()` deleted - the listings come out byte-identical. a unit test that feeds duplicate names to `print_env_var_names` would cover it. ########## core/configs/src/configs_impl/env_listing.rs: ########## @@ -0,0 +1,59 @@ +// 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 +/// call, one error policy. A closed writer (e.g. `iggy-server --list-config-env-vars | head -1`) +/// is expected, not a failure: on `BrokenPipe` this stops writing and returns `Ok(())`. Any +/// other I/O error is real and is propagated so the caller exits non-zero. +pub fn print_env_var_names<I, S, W>(names: I, writer: &mut W) -> std::io::Result<()> +where + I: IntoIterator<Item = S>, + S: Into<String>, + W: std::io::Write, +{ + let mut names: Vec<String> = names.into_iter().map(Into::into).collect(); + names.sort_unstable(); + names.dedup(); + + for name in names { + if let Err(err) = writeln!(writer, "{name}") { + return if err.kind() == std::io::ErrorKind::BrokenPipe { + Ok(()) + } else { + Err(err) + }; + } + } + Ok(()) +} + +/// Env vars `iggy-mcp --list-config-env-vars` advertises beyond the derived Review Comment: nit: this `///` attaches to `MCP_CONFIG_PATH_ENV`, not to the list it describes, and the same happens at line 51. move both onto `MCP_RUNTIME_ENV_VARS` and `CONNECTORS_RUNTIME_ENV_VARS`. ########## core/configs/src/server_config/server.rs: ########## @@ -64,6 +79,17 @@ pub const SERVER_PROCESS_ENV_VARS: &[&str] = &[ "IGGY_PASSWORD", ]; +/// Vars safe to advertise to end users via `--list-config-env-vars` — every +/// entry in `SERVER_PROCESS_ENV_VARS` except the sibling-binary/test-only +/// ones in `SERVER_SCAN_ONLY_ENV_VARS`. Computed rather than retyped, so the Review Comment: nit: this says the two views can't drift, but `SERVER_PROCESS_ENV_VARS` still retypes all seven scan-only names - dropping `IGGY_TEST_VERBOSE` from it keeps every test green. add a subset test or reword. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { + cmd.env(key, value); + } + + for arg in extra_args { + cmd.arg(arg); + } + + cmd.output().expect("listing command should run") +} + +/// Convenience wrapper over [`run_list_config_env_vars_in`] for tests that +/// don't need to inspect the working directory afterward. +fn run_list_config_env_vars<I, K, V>( + binary: &str, + env: I, + extra_args: &[&str], +) -> std::process::Output +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let directory = tempfile::tempdir().expect("temporary directory"); + run_list_config_env_vars_in(binary, directory.path(), env, extra_args) +} + +/// Convenience wrapper over [`run_list_config_env_vars`] for tests that don't +/// need to set any environment variables. +fn run_list_config_env_vars_plain(binary: &str, extra_args: &[&str]) -> std::process::Output { + run_list_config_env_vars(binary, std::iter::empty::<(&str, &str)>(), extra_args) +} + +#[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 output = run_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()), + ], + &[], + ); + + 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 baseline = run_list_config_env_vars_plain(binary, &[]); + assert_eq!( + output.stdout, baseline.stdout, + "{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" + ); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: Vec<_> = stdout.lines().collect(); + assert!(!names.is_empty(), "{binary} returned no variables"); + 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() { + // Verify that vector fields are correctly represented with <N> placeholder + let output = run_list_config_env_vars_plain("iggy-server", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Cluster nodes should have indexed templates + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_")), + "server should list cluster node index templates" + ); + + // Verify nested vector expansion (nested <N> placeholders) + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_ADVERTISED_ADDRESSES_<N>_")), + "server should list nested vector templates" + ); +} + +#[test] +fn config_env_listing_includes_connector_templates() { + // Verify that connector SINK/SOURCE templates are correctly formatted + let output = run_list_config_env_vars_plain("iggy-connectors", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Both SINK and SOURCE templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_")), + "connectors should list SINK templates with <KEY> placeholder" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_")), + "connectors should list SOURCE templates with <KEY> placeholder" + ); + + // Plugin config templates should be present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SINK plugin config templates" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_<FIELD>")), + "connectors should list SOURCE plugin config templates" + ); + assert!( + names.contains("IGGY_CONNECTORS_SINK_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SINK plugin config format" + ); + assert!( + names.contains("IGGY_CONNECTORS_SOURCE_<KEY>_PLUGIN_CONFIG_FORMAT"), + "connectors should list the typed SOURCE plugin config format" + ); + + // Verify SINK and SOURCE ENABLED fields are present + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SINK_<KEY>_ENABLED")), + "connectors should list SINK_<KEY>_ENABLED" + ); + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CONNECTORS_SOURCE_<KEY>_ENABLED")), + "connectors should list SOURCE_<KEY>_ENABLED" + ); + + assert_eq!( + stdout + .lines() + .filter(|name| *name == "IGGY_CONNECTORS_CONNECTORS_CONFIG_TYPE") + .count(), + 1, + "enum variants should produce one deduplicated tag name" + ); +} + +#[test] +fn config_env_listing_excludes_other_processes_variables() { Review Comment: nit: this never checks the exit status, and empty stdout passes every assert here - a server that prints nothing and exits 3 still goes green. assert success and non-empty stdout in `run_list_config_env_vars_in`. ########## core/connectors/runtime/src/main.rs: ########## @@ -67,7 +72,24 @@ static GLOBAL: MiMalloc = MiMalloc; #[derive(Parser, Debug)] #[command(author = "Apache Iggy", version)] -struct Args {} +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 stream fields) +- <KEY> represents connector keys (uppercased from config). + Overrides via <KEY> require the local connectors provider. +- <FIELD> represents plugin configuration field names, excluding Review Comment: nit: the help doesn't say that `<FIELD>` sets one lowercased top-level `plugin_config` key - `A_B` becomes `a_b`, never nested `a.b`. add that, plus that env can't set nested keys. ########## core/ai/mcp/src/main.rs: ########## @@ -42,7 +46,20 @@ const VERSION: &str = env!("CARGO_PKG_VERSION"); const DEFAULT_CONFIG_PATH: &str = "core/ai/mcp/config.toml"; +#[derive(Debug, Parser)] +#[command(author = "Apache Iggy", version)] +struct Args { + /// Print supported configuration environment variables and exit. + #[arg(long)] + list_config_env_vars: bool, +} + fn main() -> Result<(), McpRuntimeError> { + let args = Args::parse(); Review Comment: nit: `iggy-mcp` now exits 2 on any unknown arg (master ignored them), so a client config or `docker run` with stray args breaks. mention it in the MCP README next to the Claude Desktop example. ########## core/configs/src/server_config/cluster.rs: ########## @@ -1569,6 +1569,16 @@ mod tests { "selector env expansion must stop at max_elements = 16" ); } + + #[test] + fn advertised_addresses_env_template_keeps_each_vector_limit() { Review Comment: nit: no unit test pins a two-index name to its path, so only the cluster integration tests guard `expand`. assert here that `NODES_3_ADVERTISED_ADDRESSES_7_CLIENT_CIDR` maps to `nodes.3.advertised_addresses.7.client_cidr`. ########## core/configs_derive/src/config_env.rs: ########## @@ -255,108 +261,67 @@ fn generate_struct_impl( let (mappings, nested_fields) = collect_mappings(&fields); let const_definitions = generate_const_definitions(&mappings, prefix_str); - let mapping_entries = generate_mapping_entries(&mappings); let builder_methods = generate_builder_methods(&mappings, prefix_str); let builder_name = format_ident!("{}EnvBuilder", struct_name); - let mappings_count = mappings.len(); - // Generate code to include nested type mappings - let nested_extends: Vec<TokenStream2> = nested_fields - .iter() - .map(|info| { - let ty = &info.element_type; - let field_env_segment = &info.field_env_segment; - let field_name = &info.field_name; - - if info.is_vec { - // For Vec fields, expand with array indices - let max_elements = info.max_elements; - quote! { - let nested_mappings = <#ty as configs::ConfigEnvMappings>::env_mappings(); - for i in 0..#max_elements { - for mapping in nested_mappings { - // Transform env_name by prepending FIELD_INDEX_ - let new_env_name: &'static str = Box::leak( - format!("{}_{}_{}", #field_env_segment, i, mapping.env_name).into_boxed_str() - ); - // Transform config_path by prepending field.index. - let new_config_path: &'static str = Box::leak( - format!("{}.{}.{}", #field_name, i, mapping.config_path).into_boxed_str() - ); - all_mappings.push(configs::EnvVarMapping { - env_name: new_env_name, - config_path: new_config_path, - is_secret: mapping.is_secret, - }); - } - } + let own_template_entries = mappings.iter().map(|mapping| { + let env_suffix = &mapping.env_suffix; + let config_path = &mapping.config_path; + let is_secret = mapping.is_secret; + quote! { + configs::EnvVarTemplate { + env_name: #env_suffix, + config_path: #config_path, + is_secret: #is_secret, + max_elements: &[], + } + } + }); + let nested_template_extends = nested_fields.iter().map(|info| { + let ty = &info.element_type; + let segment = &info.field_env_segment; + let field_name = &info.field_name; + if info.is_vec { + let max_elements = info.max_elements; + quote! { + for template in <#ty as configs::ConfigEnvMappings>::env_templates() { + let env_name = Box::leak(format!("{}_<N>_{}", #segment, template.env_name).into_boxed_str()); + let config_path = Box::leak(format!("{}.<N>.{}", #field_name, template.config_path).into_boxed_str()); + let limits = Box::leak(std::iter::once(#max_elements) + .chain(template.max_elements.iter().copied()) + .collect::<Vec<_>>() + .into_boxed_slice()); + all_templates.push(configs::EnvVarTemplate { + env_name, + config_path, + is_secret: template.is_secret, + max_elements: limits, + }); } - } else { - // For non-Vec nested types, transform by prepending field name - quote! { - let nested_mappings = <#ty as configs::ConfigEnvMappings>::env_mappings(); - for mapping in nested_mappings { - // Transform env_name by prepending FIELD_ - let new_env_name: &'static str = Box::leak( - format!("{}_{}", #field_env_segment, mapping.env_name).into_boxed_str() - ); - // Transform config_path by prepending field. - let new_config_path: &'static str = Box::leak( - format!("{}.{}", #field_name, mapping.config_path).into_boxed_str() - ); - all_mappings.push(configs::EnvVarMapping { - env_name: new_env_name, - config_path: new_config_path, - is_secret: mapping.is_secret, - }); - } + } + } else { + quote! { + for template in <#ty as configs::ConfigEnvMappings>::env_templates() { + let env_name = Box::leak(format!("{}_{}", #segment, template.env_name).into_boxed_str()); + let config_path = Box::leak(format!("{}.{}", #field_name, template.config_path).into_boxed_str()); + all_templates.push(configs::EnvVarTemplate { + env_name, + config_path, + is_secret: template.is_secret, + max_elements: template.max_elements, + }); } } - }) - .collect(); - - let prefix_application = if has_prefix { - quote! { - let all_mappings: Vec<configs::EnvVarMapping> = all_mappings - .into_iter() - .map(|m| configs::EnvVarMapping { - env_name: Box::leak(format!("{}{}", #prefix_str, m.env_name).into_boxed_str()), - config_path: m.config_path, - is_secret: m.is_secret, - }) - .collect(); } - } else { - quote! {} - }; - - // If there are nested types, we need to use OnceLock to combine mappings - let env_mappings_impl = if nested_fields.is_empty() && !has_prefix { - // Simple case: no nested fields, no prefix transformation needed + }); + let template_prefix_application = if has_prefix { Review Comment: simplification: the prefix is known at macro time (`generate_const_definitions` already uses it), so bake it into the literals and drop this runtime pass that leaks a prefixed copy of every name. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { + cmd.env(key, value); + } + + for arg in extra_args { + cmd.arg(arg); + } + + cmd.output().expect("listing command should run") +} + +/// Convenience wrapper over [`run_list_config_env_vars_in`] for tests that +/// don't need to inspect the working directory afterward. +fn run_list_config_env_vars<I, K, V>( + binary: &str, + env: I, + extra_args: &[&str], +) -> std::process::Output +where + I: IntoIterator<Item = (K, V)>, + K: AsRef<std::ffi::OsStr>, + V: AsRef<std::ffi::OsStr>, +{ + let directory = tempfile::tempdir().expect("temporary directory"); + run_list_config_env_vars_in(binary, directory.path(), env, extra_args) +} + +/// Convenience wrapper over [`run_list_config_env_vars`] for tests that don't +/// need to set any environment variables. +fn run_list_config_env_vars_plain(binary: &str, extra_args: &[&str]) -> std::process::Output { + run_list_config_env_vars(binary, std::iter::empty::<(&str, &str)>(), extra_args) +} + +#[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 output = run_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()), + ], + &[], + ); + + 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 baseline = run_list_config_env_vars_plain(binary, &[]); + assert_eq!( + output.stdout, baseline.stdout, + "{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" + ); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: Vec<_> = stdout.lines().collect(); + assert!(!names.is_empty(), "{binary} returned no variables"); + 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() { + // Verify that vector fields are correctly represented with <N> placeholder + let output = run_list_config_env_vars_plain("iggy-server", &[]); + + let stdout = String::from_utf8(output.stdout).expect("UTF-8 output"); + let names: HashSet<_> = stdout.lines().collect(); + + // Cluster nodes should have indexed templates + assert!( + names + .iter() + .any(|name| name.contains("IGGY_CLUSTER_NODES_<N>_")), Review Comment: simplification: the checks at lines 167, 192 and 198 are implied by the longer ones at 175, 216 and 220 - drop them. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( Review Comment: simplification: three wrappers call each other and five tests decode stdout by hand. one helper that runs the binary, asserts success and empty stderr, and returns the lines cuts about 40 lines. ########## core/integration/tests/config_env_listing/mod.rs: ########## @@ -0,0 +1,351 @@ +// 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); + +/// Runs `binary --list-config-env-vars` (plus `extra_args`) with `env` applied, +/// isolated to `directory`. Shared by every test below so each only states +/// what differs: the binary, the env it sets, and any extra flags. +fn run_list_config_env_vars_in<I, K, V>( + binary: &str, + directory: &std::path::Path, + env: I, + extra_args: &[&str], +) -> std::process::Output +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") + .timeout(LIST_ENV_VARS_TIMEOUT); + + for (key, value) in env { Review Comment: simplification: `assert_cmd`'s `Command` has `envs` and `args`, so these two loops can be `.envs(env).args(extra_args)` on the builder. -- 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]
