numinnex commented on code in PR #4350:
URL: https://github.com/apache/iggy/pull/4350#discussion_r4184474562
##########
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:
nit: `"SINK"`/`"SOURCE"` duplicate `connector_type()`
(local_provider.rs:393) in uppercase, and the "can't drift" comment below only
covers the prefix. derive them from it. also import `connector_env_prefix` at
the top instead of the inline full path, `local_provider` became `pub(crate)`
only for this call.
##########
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:
this advertises `IGGY_CONNECTORS_{SINK,SOURCE}_<KEY>_KEY` and `_VERSION` as
supported, and both are harmful to set. With sink files foo v0, foo v1, bar v0:
- `..._SINK_FOO_VERSION=5` collapses foo to `[5]`, one file is silently
dropped (local_provider.rs:239) and `get_sink_config(foo, Some(0))` returns
`Ok(None)`
- `..._SINK_BAR_KEY=renamed` makes bar reappear as `renamed`, while plugin
overrides and the scan still use the file key (local_provider.rs:350,841)
master already reads them, but listing them invites it. filter
`key`/`version` out here and assert in the integration test that they are
absent.
##########
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:
nit: "Add to the runtime env-var lists" names no list, and connectors/MCP
have no scan-only list, so the hint pushes devs to advertise internal vars.
name the consts, or give them the same scan-only split the server has.
##########
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:
nit: this is pub and re-exported, and every direct call leaks two strings
per mapping. it can't go private since macro output calls it, so
`#[doc(hidden)]` at least keeps it off the public surface.
##########
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:
nit: nothing checks the `<N>` count in `env_name` against
`max_elements.len()` on this pub type. too few limits leaves a literal `<N>` in
the name, too many leaks 256 duplicate copies. a `debug_assert_eq!` would catch
it.
##########
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:
nit: `"IGGY_DISPLAY_CONFIG"` is retyped here while MCP and connectors reuse
`DISPLAY_CONFIG_ENV`. needs a `pub(crate) use` re-export since `file_provider`
is private.
##########
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:
nit: the 0-255 / 0-15 bounds are hardcoded here and in the connectors help
(main.rs:83), and nothing ties them to `max_elements`. a test comparing them
with `env_templates()` would keep them honest. also, "plugins" at :101 doesn't
apply to the server.
##########
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:
nit: em dash in the rustdoc, CLAUDE.md rule 4.
##########
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:
nit: this sentence splits "This Claude Desktop example uses ...:" from its
JSON block. move it above line 65 or below the example. nothing in the
integration test pins the exit-2 behavior either.
##########
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:
nit: still implied by the `IGGY_CLUSTER_NODES_<N>_ADVERTISED_ADDRESSES_<N>_`
check below (the :143 thread). drop it, and the WHAT-comments at :139 and :147.
--
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]
