From 8630c12d415c73f67acd428456b05ee6f092ab7f Mon Sep 17 00:00:00 2001 From: xeniape Date: Mon, 19 Aug 2024 14:35:23 +0200 Subject: [PATCH 1/6] change evaluation order of product configurations --- .../src/product_config_utils.rs | 113 ++++++++++-------- 1 file changed, 63 insertions(+), 50 deletions(-) diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 70d14f97b..b59fae9b4 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -345,7 +345,7 @@ fn process_validation_result( /// with the highest priority. The merge priority chain looks like this where '->' means /// "overwrites if existing or adds": /// -/// group overrides -> group config -> role overrides -> role config (TODO: -> common_config) +/// group overrides -> role overrides -> group config -> role config (TODO: -> common_config) /// /// The output is a map where the [`crate::role_utils::RoleGroup] name points to another map of /// [`product_config::types::PropertyValidationResult`] that points to the mapped configuration @@ -368,24 +368,43 @@ where { let mut result = HashMap::new(); + // Properties from the role have the lowest priority, so they are computed first... let role_properties = parse_role_config(resource, role_name, &role.config, property_kinds)?; + let role_overrides = parse_role_overrides(&role.config, property_kinds)?; // for each role group ... for (role_group_name, role_group) in &role.role_groups { - // ... compute the group properties ... + let mut rg_properties_merged = role_properties.clone(); + + // ... compute the group properties and merge them into role properties. let role_group_properties = parse_role_config(resource, role_name, &role_group.config, property_kinds)?; - - // ... and merge them with the role properties. - let mut role_properties_copy = role_properties.clone(); for (property_kind, properties) in role_group_properties { - role_properties_copy + rg_properties_merged .entry(property_kind) .or_default() .extend(properties); } - result.insert(role_group_name.clone(), role_properties_copy); + // ... copy role overrides and merge them into `rg_properties_merged`. + let role_overrides_copy = role_overrides.clone(); + for (property_kind, property_overrides) in role_overrides_copy { + rg_properties_merged + .entry(property_kind) + .or_default() + .extend(property_overrides); + } + + // ... compute the role group overrides and merge them into `rg_properties_merged`. + let role_group_overrides = parse_role_overrides(&role_group.config, property_kinds)?; + for (property_kind, property_overrides) in role_group_overrides { + rg_properties_merged + .entry(property_kind) + .or_default() + .extend(property_overrides); + } + + result.insert(role_group_name.clone(), rg_properties_merged); } Ok(result) @@ -411,20 +430,19 @@ where T: Configuration, { let mut result = HashMap::new(); - for property_kind in property_kinds { match property_kind { PropertyNameKind::File(file) => result.insert( property_kind.clone(), - parse_file_properties(resource, role_name, config, file)?, + config.config.compute_files(resource, role_name, file)?, ), PropertyNameKind::Env => result.insert( property_kind.clone(), - parse_env_properties(resource, role_name, config)?, + config.config.compute_env(resource, role_name)?, ), PropertyNameKind::Cli => result.insert( property_kind.clone(), - parse_cli_properties(resource, role_name, config)?, + config.config.compute_cli(resource, role_name)?, ), }; } @@ -432,65 +450,60 @@ where Ok(result) } -fn parse_cli_properties( - resource: &::Configurable, - role_name: &str, +fn parse_role_overrides( config: &CommonConfiguration, -) -> Result>> -where - T: Configuration, -{ - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_cli(resource, role_name)?; - - // ...followed by config_overrides from the role - for (key, value) in &config.cli_overrides { - final_properties.insert(key.clone(), Some(value.clone())); - } - - Ok(final_properties) -} - -fn parse_env_properties( - resource: &::Configurable, - role_name: &str, - config: &CommonConfiguration, -) -> Result>> + property_kinds: &[PropertyNameKind], +) -> Result>>> where T: Configuration, { - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_env(resource, role_name)?; - - // ...followed by config_overrides from the role - for (key, value) in &config.env_overrides { - final_properties.insert(key.clone(), Some(value.clone())); + let mut result = HashMap::new(); + for property_kind in property_kinds { + match property_kind { + PropertyNameKind::File(file) => { + result.insert(property_kind.clone(), parse_file_overrides(config, file)?) + } + PropertyNameKind::Env => result.insert( + property_kind.clone(), + config + .env_overrides + .clone() + .into_iter() + .map(|(k, v)| (k, Some(v))) + .collect(), + ), + PropertyNameKind::Cli => result.insert( + property_kind.clone(), + config + .cli_overrides + .clone() + .into_iter() + .map(|(k, v)| (k, Some(v))) + .collect(), + ), + }; } - Ok(final_properties) + Ok(result) } -fn parse_file_properties( - resource: &::Configurable, - role_name: &str, +fn parse_file_overrides( config: &CommonConfiguration, file: &str, ) -> Result>> where T: Configuration, { - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_files(resource, role_name, file)?; + let mut final_overrides: BTreeMap> = BTreeMap::new(); - // ...followed by config_overrides from the role // For Conf files only process overrides that match our file name if let Some(config) = config.config_overrides.get(file) { for (key, value) in config { - final_properties.insert(key.clone(), Some(value.clone())); + final_overrides.insert(key.clone(), Some(value.clone())); } } - Ok(final_properties) + Ok(final_overrides) } #[cfg(test)] @@ -1085,7 +1098,7 @@ mod tests { ), PropertyNameKind::Cli => collection!( - "cli".to_string() => Some(GROUP_CLI.to_string()), + "cli".to_string() => Some("cli".to_string()), ), } }; From 73d54200be740dd7bb488b4b5d64afc50f79bb34 Mon Sep 17 00:00:00 2001 From: xeniape Date: Mon, 19 Aug 2024 15:07:02 +0200 Subject: [PATCH 2/6] add changelog --- crates/stackable-operator/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 7e58a23d8..2c2e5a5d4 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -8,7 +8,12 @@ All notable changes to this project will be documented in this file. - `iter::reverse_if` helper ([#838]). +### Changed + +- BREAKING: Swap priority order of role group config and role overrides in configuration merging to prioritize overrides in general ([#841]). + [#838]: https://github.com/stackabletech/operator-rs/pull/838 +[#841]: https://github.com/stackabletech/operator-rs/pull/841 ## [0.73.0] - 2024-08-09 From 3a2b94acd565264063f76b75eec4c14945ddfaba Mon Sep 17 00:00:00 2001 From: xeniape Date: Tue, 20 Aug 2024 14:25:01 +0200 Subject: [PATCH 3/6] rename variable and move clone --- .../src/product_config_utils.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index b59fae9b4..0a52a3903 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -374,37 +374,36 @@ where // for each role group ... for (role_group_name, role_group) in &role.role_groups { - let mut rg_properties_merged = role_properties.clone(); + let mut role_group_properties_merged = role_properties.clone(); // ... compute the group properties and merge them into role properties. let role_group_properties = parse_role_config(resource, role_name, &role_group.config, property_kinds)?; for (property_kind, properties) in role_group_properties { - rg_properties_merged + role_group_properties_merged .entry(property_kind) .or_default() .extend(properties); } - // ... copy role overrides and merge them into `rg_properties_merged`. - let role_overrides_copy = role_overrides.clone(); - for (property_kind, property_overrides) in role_overrides_copy { - rg_properties_merged + // ... copy role overrides and merge them into `role_group_properties_merged`. + for (property_kind, property_overrides) in role_overrides.clone() { + role_group_properties_merged .entry(property_kind) .or_default() .extend(property_overrides); } - // ... compute the role group overrides and merge them into `rg_properties_merged`. + // ... compute the role group overrides and merge them into `role_group_properties_merged`. let role_group_overrides = parse_role_overrides(&role_group.config, property_kinds)?; for (property_kind, property_overrides) in role_group_overrides { - rg_properties_merged + role_group_properties_merged .entry(property_kind) .or_default() .extend(property_overrides); } - result.insert(role_group_name.clone(), rg_properties_merged); + result.insert(role_group_name.clone(), role_group_properties_merged); } Ok(result) From ffd77f80aa434f86f58707486db4b25cc8b69568 Mon Sep 17 00:00:00 2001 From: xeniape Date: Wed, 21 Aug 2024 17:04:36 +0200 Subject: [PATCH 4/6] add unit test for config override order --- .../src/product_config_utils.rs | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 0a52a3903..3e7bb388a 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -1058,6 +1058,140 @@ mod tests { assert_eq!(config, expected); } + #[rstest] + #[case( + HashMap::from([ + ("env".to_string(), ROLE_ENV_OVERRIDE.to_string()), + ]), + HashMap::from([ + ("env".to_string(), GROUP_ENV_OVERRIDE.to_string()), + ]), + BTreeMap::from([ + ("cli".to_string(), ROLE_CLI_OVERRIDE.to_string()), + ]), + BTreeMap::from([ + ("cli".to_string(), GROUP_CLI_OVERRIDE.to_string()), + ]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), ROLE_CONF_OVERRIDE.to_string()) + ])) + ]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), GROUP_CONF_OVERRIDE.to_string()) + ])) + ]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(GROUP_ENV_OVERRIDE.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(GROUP_CLI_OVERRIDE.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(GROUP_CONF_OVERRIDE.to_string()), + } + } + } + )] + #[case( + HashMap::from([ + ("env".to_string(), ROLE_ENV_OVERRIDE.to_string()), + ]), + HashMap::from([]), + BTreeMap::from([ + ("cli".to_string(), ROLE_CLI_OVERRIDE.to_string()), + ]), + BTreeMap::from([]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), ROLE_CONF_OVERRIDE.to_string()) + ])) + ]), + HashMap::from([]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(ROLE_ENV_OVERRIDE.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(ROLE_CLI_OVERRIDE.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(ROLE_CONF_OVERRIDE.to_string()), + } + } + } + )] + #[case( + HashMap::from([]), + HashMap::from([]), + BTreeMap::from([]), + BTreeMap::from([]), + HashMap::from([]), + HashMap::from([]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(GROUP_ENV.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(GROUP_CLI.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(GROUP_CONFIG.to_string()), + } + } + } + )] + fn test_order_in_transform_role_to_config( + #[case] role_env_override: HashMap, + #[case] group_env_override: HashMap, + #[case] role_cli_override: BTreeMap, + #[case] group_cli_override: BTreeMap, + #[case] role_conf_override: HashMap>, + #[case] group_conf_override: HashMap>, + #[case] expected: HashMap< + String, + HashMap>>, + >, + ) { + let role: Role, TestRoleConfig> = Role { + config: build_common_config( + Some(Box::new(TestConfig { + conf: Some(ROLE_CONFIG.to_string()), + env: Some(ROLE_ENV.to_string()), + cli: Some(ROLE_CLI.to_string()), + })), + Some(role_conf_override), + Some(role_env_override), + Some(role_cli_override), + ), + role_config: Default::default(), + role_groups: collection! {"role_group".to_string() => RoleGroup { + replicas: Some(1), + config: build_common_config( + build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), + Some(group_conf_override), + Some(group_env_override), + Some(group_cli_override)), + }}, + }; + + let property_kinds = vec![ + PropertyNameKind::Env, + PropertyNameKind::Cli, + PropertyNameKind::File("file".to_string()), + ]; + + let config = + transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds).unwrap(); + + assert_eq!(config, expected); + } + #[test] fn test_transform_role_to_config_overrides() { let role_group = "role_group"; From 9bce51d470d7d292aed7743b8b62c733a514aabc Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 22 Aug 2024 11:14:08 +0200 Subject: [PATCH 5/6] remove dublicate heading --- crates/stackable-operator/CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 131f2338f..444945686 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -12,9 +12,6 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Replace `lazy_static` with `std::cell::LazyCell` (the original implementation was done in [#827] and reverted in [#835]) ([#840]). - -### Changed - - BREAKING: Swap priority order of role group config and role overrides in configuration merging to prioritize overrides in general ([#841]). [#838]: https://github.com/stackabletech/operator-rs/pull/838 From cce5643eb23c1c42b00a754cfc5b56dba4947134 Mon Sep 17 00:00:00 2001 From: Xenia Date: Thu, 22 Aug 2024 11:22:14 +0200 Subject: [PATCH 6/6] Update crates/stackable-operator/src/product_config_utils.rs Co-authored-by: Techassi --- crates/stackable-operator/src/product_config_utils.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index de3a51972..47f281614 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -1163,11 +1163,7 @@ mod tests { ) { let role: Role, TestRoleConfig> = Role { config: build_common_config( - Some(Box::new(TestConfig { - conf: Some(ROLE_CONFIG.to_string()), - env: Some(ROLE_ENV.to_string()), - cli: Some(ROLE_CLI.to_string()), - })), + build_test_config(ROLE_CONFIG, ROLE_ENV, ROLE_CLI), Some(role_conf_override), Some(role_env_override), Some(role_cli_override),