github-actions[bot] commented on code in PR #4486:
URL: https://github.com/apache/iggy/pull/4486#discussion_r4237894070
##########
core/connectors/runtime/src/configs/connectors.rs:
##########
@@ -176,8 +176,11 @@ impl CreateSinkConfig {
#[derive(Debug, Default, Clone, Serialize, Deserialize, ConfigEnv)]
pub struct SinkConfig {
+ // Identity comes from the file: an env KEY would orphan saved state.
+ #[config_env(skip)]
Review Comment:
warning: `IGGY_CONNECTORS_..._KEY` and `..._VERSION` now reach the
unknown-variable scan, which makes a debug build panic at
`typed_env_provider.rs:323` and warns with a field-name remedy in release.
Treat both names as known-but-ignored and log that identity comes from the
config file. Also at lines 183, 236, 239.
##########
core/connectors/runtime/src/configs/connectors.rs:
##########
@@ -176,8 +176,11 @@ impl CreateSinkConfig {
#[derive(Debug, Default, Clone, Serialize, Deserialize, ConfigEnv)]
pub struct SinkConfig {
+ // Identity comes from the file: an env KEY would orphan saved state.
Review Comment:
nit: The comment claims an env `KEY` orphans saved state, but only sources
keep state. For a sink, an env key moves the consumer group instead. Reword the
sink comment to name the identity change instead of state loss.
##########
core/connectors/runtime/src/configs/connectors.rs:
##########
@@ -488,6 +494,36 @@ impl ConnectorsConfig {
#[cfg(test)]
mod tests {
use super::*;
+ use configs::ConfigEnvMappings;
+
+ #[test]
+ fn
given_sink_and_source_configs_when_mapped_should_exclude_key_and_version() {
+ for (name, mappings, templates) in [
+ (
+ "sink",
+ SinkConfig::env_mappings(),
+ SinkConfig::env_templates(),
+ ),
+ (
+ "source",
+ SourceConfig::env_mappings(),
+ SourceConfig::env_templates(),
+ ),
+ ] {
+ assert!(
Review Comment:
nit: Both assertions are `all()`-quantified, so an empty mapping list passes
this test. Assert that a known name such as `ENABLED` is present before the
exclusion checks.
##########
core/connectors/runtime/src/configs/connectors.rs:
##########
@@ -488,6 +494,36 @@ impl ConnectorsConfig {
#[cfg(test)]
mod tests {
use super::*;
+ use configs::ConfigEnvMappings;
+
+ #[test]
+ fn
given_sink_and_source_configs_when_mapped_should_exclude_key_and_version() {
+ for (name, mappings, templates) in [
+ (
+ "sink",
+ SinkConfig::env_mappings(),
+ SinkConfig::env_templates(),
+ ),
+ (
+ "source",
+ SourceConfig::env_mappings(),
+ SourceConfig::env_templates(),
+ ),
+ ] {
+ assert!(
+ mappings
+ .iter()
+ .all(|mapping| !matches!(mapping.env_name, "KEY" |
"VERSION")),
+ "{name} maps key or version from the environment"
+ );
+ assert!(
Review Comment:
simplification: `env_mappings()` only expands `env_templates()`, so this
assertion and the one above it check the same absence twice. Keep the mappings
assertion and delete the template block, which the listing test in
`core/integration/tests/config_env_listing/mod.rs` already covers.
--
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]
