From 87070c1a6ea6b4f4f4682c473d5f0d4892d8b576 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Mon, 13 Dec 2021 14:41:53 +0100 Subject: [PATCH 1/4] Default Role/RoleGroup configuration when deserializing Fixes #281 --- Cargo.toml | 2 +- src/product_config_utils.rs | 172 +++++++++++++----------------------- src/role_utils.rs | 72 +++++++++------ 3 files changed, 109 insertions(+), 137 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9939522e2..2cb947bf8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ lazy_static = "1.4.0" product-config = { git = "https://github.com/stackabletech/product-config.git", tag = "0.3.0" } rand = "0.8.4" regex = "1.5.4" -schemars = "0.8.6" +schemars = "0.8.7" serde = "1.0.130" serde_json = "1.0.68" serde_yaml = "0.8.21" diff --git a/src/product_config_utils.rs b/src/product_config_utils.rs index fb73052d0..88709f071 100644 --- a/src/product_config_utils.rs +++ b/src/product_config_utils.rs @@ -347,18 +347,13 @@ where { let mut result = HashMap::new(); - let role_properties = - parse_role_config(resource, role_name, role.config.as_ref(), property_kinds); + let role_properties = parse_role_config(resource, role_name, &role.config, property_kinds); // for each role group ... for (role_group_name, role_group) in &role.role_groups { // ... compute the group properties ... - let role_group_properties = parse_role_config( - resource, - role_name, - role_group.config.as_ref(), - property_kinds, - ); + 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(); @@ -388,7 +383,7 @@ where fn parse_role_config( resource: &::Configurable, role_name: &str, - config: Option<&CommonConfiguration>, + config: &CommonConfiguration, property_kinds: &[PropertyNameKind], ) -> HashMap>> where @@ -418,31 +413,17 @@ where fn parse_cli_properties( resource: &::Configurable, role_name: &str, - config: Option<&CommonConfiguration>, + config: &CommonConfiguration, ) -> BTreeMap> where T: Configuration, { - let mut final_properties = BTreeMap::new(); - // Properties from the role have the lowest priority, so they are computed and added first... - if let Some(CommonConfiguration { - config: Some(ref config), - .. - }) = config - { - final_properties = config.compute_cli(resource, role_name).unwrap(); - } + let mut final_properties = config.config.compute_cli(resource, role_name).unwrap(); // ...followed by config_overrides from the role - if let Some(CommonConfiguration { - cli_overrides: Some(ref config), - .. - }) = config - { - for (key, value) in config { - final_properties.insert(key.clone(), Some(value.clone())); - } + for (key, value) in &config.cli_overrides { + final_properties.insert(key.clone(), Some(value.clone())); } final_properties @@ -451,31 +432,17 @@ where fn parse_env_properties( resource: &::Configurable, role_name: &str, - config: Option<&CommonConfiguration>, + config: &CommonConfiguration, ) -> BTreeMap> where T: Configuration, { - let mut final_properties = BTreeMap::new(); - // Properties from the role have the lowest priority, so they are computed and added first... - if let Some(CommonConfiguration { - config: Some(ref config), - .. - }) = config - { - final_properties = config.compute_env(resource, role_name).unwrap(); - } + let mut final_properties = config.config.compute_env(resource, role_name).unwrap(); // ...followed by config_overrides from the role - if let Some(CommonConfiguration { - env_overrides: Some(ref config), - .. - }) = config - { - for (key, value) in config { - final_properties.insert(key.clone(), Some(value.clone())); - } + for (key, value) in &config.env_overrides { + final_properties.insert(key.clone(), Some(value.clone())); } final_properties @@ -484,36 +451,23 @@ where fn parse_file_properties( resource: &::Configurable, role_name: &str, - config: Option<&CommonConfiguration>, + config: &CommonConfiguration, file: &str, ) -> BTreeMap> where T: Configuration, { - let mut final_properties = BTreeMap::new(); - // Properties from the role have the lowest priority, so they are computed and added first... - if let Some(CommonConfiguration { - config: Some(ref inner_config), - .. - }) = config - { - final_properties = inner_config - .compute_files(resource, role_name, file) - .unwrap(); - } + let mut final_properties = config + .config + .compute_files(resource, role_name, file) + .unwrap(); // ...followed by config_overrides from the role - if let Some(CommonConfiguration { - config_overrides: Some(ref inner_config), - .. - }) = config - { - // For Conf files only process overrides that match our file name - if let Some(config) = inner_config.get(file) { - for (key, value) in config { - final_properties.insert(key.clone(), Some(value.clone())); - } + // 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())); } } @@ -557,7 +511,7 @@ mod tests { const GROUP_ENV_OVERRIDE: &str = "group_env_override"; const GROUP_CLI_OVERRIDE: &str = "group_cli_override"; - #[derive(Clone, Debug, PartialEq)] + #[derive(Clone, Default, Debug, PartialEq)] struct TestConfig { pub conf: Option, pub env: Option, @@ -618,13 +572,13 @@ mod tests { config_overrides: Option>>, env_overrides: Option>, cli_overrides: Option>, - ) -> Option>> { - Some(CommonConfiguration { - config: test_config, - config_overrides, - env_overrides, - cli_overrides, - }) + ) -> CommonConfiguration> { + CommonConfiguration { + config: test_config.unwrap_or_default(), + config_overrides: config_overrides.unwrap_or_default(), + env_overrides: env_overrides.unwrap_or_default(), + cli_overrides: cli_overrides.unwrap_or_default(), + } } fn build_config_override( @@ -745,7 +699,7 @@ mod tests { ), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), - config: None, + config: CommonConfiguration::default(), selector: None, }}, }, @@ -776,7 +730,7 @@ mod tests { ), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), - config: None, + config: CommonConfiguration::default(), selector: None, }}, }, @@ -815,7 +769,7 @@ mod tests { }}, }, (false, true, false, true) => Role { - config: None, + config: CommonConfiguration::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -827,7 +781,7 @@ mod tests { }}, }, (false, true, false, false) => Role { - config: None, + config: CommonConfiguration::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -864,12 +818,12 @@ mod tests { ), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), - config: None, + config: CommonConfiguration::default(), selector: None, }}, }, (false, false, false, true) => Role { - config: None, + config: CommonConfiguration::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -881,10 +835,10 @@ mod tests { }}, }, (false, false, false, false) => Role { - config: None, + config: CommonConfiguration::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), - config: None, + config: CommonConfiguration::default(), selector: None, }}, }, @@ -1226,34 +1180,34 @@ mod tests { let roles: HashMap, Role>)> = collection! { role_1.to_string() => (vec![PropertyNameKind::File(file_name.to_string()), PropertyNameKind::Env], Role { - config: None, - role_groups: collection! { - role_group_1.to_string() => RoleGroup { - replicas: Some(1), - config: build_common_config( - build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), - None, - None, - None - ), - selector: None, - }} - } + config: CommonConfiguration::default(), + role_groups: collection! { + role_group_1.to_string() => RoleGroup { + replicas: Some(1), + config: build_common_config( + build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), + None, + None, + None + ), + selector: None, + }} + } ), role_2.to_string() => (vec![PropertyNameKind::File(file_name.to_string())], Role { - config: None, - role_groups: collection! { - role_group_2.to_string() => RoleGroup { - replicas: Some(1), - config: build_common_config( - build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), - None, - None, - None - ), - selector: None, - }} - } + config: CommonConfiguration::default(), + role_groups: collection! { + role_group_2.to_string() => RoleGroup { + replicas: Some(1), + config: build_common_config( + build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), + None, + None, + None + ), + selector: None, + }} + } ), }; diff --git a/src/role_utils.rs b/src/role_utils.rs index 239d4cc51..398b09ce0 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -100,21 +100,39 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use tracing::{debug, trace}; -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde( + rename_all = "camelCase", + bound(deserialize = "T: Default + Deserialize<'de>") +)] pub struct CommonConfiguration { - pub config: Option, - pub config_overrides: Option>>, - pub env_overrides: Option>, + #[serde(default)] + // We can't depend on T being `Default`, since that trait is not object-safe + // We only need to generate schemas for fully specified types, but schemars_derive + // does not support specifying custom bounds. + #[schemars(default = "config_schema_default")] + pub config: T, + #[serde(default)] + pub config_overrides: HashMap>, + #[serde(default)] + pub env_overrides: HashMap, // BTreeMap to keep some order with the cli arguments. - pub cli_overrides: Option>, + #[serde(default)] + pub cli_overrides: BTreeMap, +} + +fn config_schema_default() -> serde_json::Value { + serde_json::json!({}) } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] +#[serde( + rename_all = "camelCase", + bound(deserialize = "T: Default + Deserialize<'de>") +)] pub struct Role { #[serde(flatten)] - pub config: Option>, + pub config: CommonConfiguration, pub role_groups: HashMap>, } @@ -126,14 +144,13 @@ impl Role { /// have different structs implementing Configuration. pub fn erase(self) -> Role>> { Role { - config: self.config.map(|common| CommonConfiguration { - config: common.config.map(|cfg| { - Box::new(cfg) as Box> - }), - config_overrides: common.config_overrides, - env_overrides: common.env_overrides, - cli_overrides: common.cli_overrides, - }), + config: CommonConfiguration { + config: Box::new(self.config.config) + as Box>, + config_overrides: self.config.config_overrides, + env_overrides: self.config.env_overrides, + cli_overrides: self.config.cli_overrides, + }, role_groups: self .role_groups .into_iter() @@ -141,15 +158,13 @@ impl Role { ( name, RoleGroup { - config: group.config.map(|common| CommonConfiguration { - config: common.config.map(|cfg| { - Box::new(cfg) - as Box> - }), - config_overrides: common.config_overrides, - env_overrides: common.env_overrides, - cli_overrides: common.cli_overrides, - }), + config: CommonConfiguration { + config: Box::new(group.config.config) + as Box>, + config_overrides: group.config.config_overrides, + env_overrides: group.config.env_overrides, + cli_overrides: group.config.cli_overrides, + }, replicas: group.replicas, selector: group.selector, }, @@ -161,10 +176,13 @@ impl Role { } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] +#[serde( + rename_all = "camelCase", + bound(deserialize = "T: Default + Deserialize<'de>") +)] pub struct RoleGroup { #[serde(flatten)] - pub config: Option>, + pub config: CommonConfiguration, pub replicas: Option, pub selector: Option, } From e7d3d74047dd8621998cb79dbfe124c97cc7dcf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Mon, 13 Dec 2021 14:46:37 +0100 Subject: [PATCH 2/4] Changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96fa3bfb7..08245e666 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed +- BREAKING: `Configuration::compute_*` are now invoked even when `config` field is not provided on `Role`/`RoleGroup` ([#282]). + - `CommonConfiguration::config` is no longer `Option`al + - `Role::config` is no longer `Option`al + - `RoleGroup::config` is no longer `Option`al + +[#282]: https://github.com/stackabletech/operator-rs/pull/282 + ## [0.6.0] - 2021-12-13 ### Changed From fed083dd95e7f904cf9406a19aba0c434b6cacb1 Mon Sep 17 00:00:00 2001 From: maltesander Date: Thu, 16 Dec 2021 10:42:02 +0100 Subject: [PATCH 3/4] Proper error handling for parse_* methods. --- CHANGELOG.md | 1 + src/product_config_utils.rs | 75 ++++++++++++++++++++----------------- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 711eb0a38..78388931f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Changed +- BREAKING: Introduced proper error handling for `parse_cli_properties`, `parse_env_properties` and `parse_file_properties` ([#282]). - BREAKING: `Configuration::compute_*` are now invoked even when `config` field is not provided on `Role`/`RoleGroup` ([#282]). - `CommonConfiguration::config` is no longer `Option`al - `Role::config` is no longer `Option`al diff --git a/src/product_config_utils.rs b/src/product_config_utils.rs index 88709f071..31bacfb49 100644 --- a/src/product_config_utils.rs +++ b/src/product_config_utils.rs @@ -18,6 +18,8 @@ pub enum ConfigError { }, } +pub type ConfigResult = std::result::Result; + /// This trait is used to compute configuration properties for products. /// /// This needs to be implemented for every T in the [`crate::role_utils::CommonConfiguration`] struct @@ -44,20 +46,20 @@ pub trait Configuration { &self, resource: &Self::Configurable, role_name: &str, - ) -> Result>, ConfigError>; + ) -> ConfigResult>>; fn compute_cli( &self, resource: &Self::Configurable, role_name: &str, - ) -> Result>, ConfigError>; + ) -> ConfigResult>>; fn compute_files( &self, resource: &Self::Configurable, role_name: &str, file: &str, - ) -> Result>, ConfigError>; + ) -> ConfigResult>>; } impl Configuration for Box { @@ -67,7 +69,7 @@ impl Configuration for Box { &self, resource: &Self::Configurable, role_name: &str, - ) -> Result>, ConfigError> { + ) -> ConfigResult>> { T::compute_env(self, resource, role_name) } @@ -75,7 +77,7 @@ impl Configuration for Box { &self, resource: &Self::Configurable, role_name: &str, - ) -> Result>, ConfigError> { + ) -> ConfigResult>> { T::compute_cli(self, resource, role_name) } @@ -84,7 +86,7 @@ impl Configuration for Box { resource: &Self::Configurable, role_name: &str, file: &str, - ) -> Result>, ConfigError> { + ) -> ConfigResult>> { T::compute_files(self, resource, role_name, file) } } @@ -94,6 +96,11 @@ impl Configuration for Box { pub type RoleConfigByPropertyKind = HashMap>>>>; +/// Type to sort config properties via kind (files, env, cli) and via groups. +/// HashMap>> +pub type RoleGroupConfigByPropertyKind = + HashMap>>>; + /// Type to sort config properties via kind (files, env, cli), via groups and via roles. This /// is the validated output to be used in other operators. The difference to [`RoleConfigByPropertyKind`] /// is that the properties BTreeMap does not contain any options. @@ -146,7 +153,7 @@ pub fn config_for_role_and_group<'a>( pub fn transform_all_roles_to_config( resource: &T::Configurable, roles: HashMap, Role)>, -) -> RoleConfigByPropertyKind +) -> ConfigResult where T: Configuration, { @@ -154,11 +161,11 @@ where for (role_name, (property_name_kinds, role)) in &roles { let role_properties = - transform_role_to_config(resource, role_name, role, property_name_kinds); + transform_role_to_config(resource, role_name, role, property_name_kinds)?; result.insert(role_name.to_string(), role_properties); } - result + Ok(result) } /// Validates a product configuration for all roles and role_groups. Requires a valid product config @@ -341,19 +348,19 @@ fn transform_role_to_config( role_name: &str, role: &Role, property_kinds: &[PropertyNameKind], -) -> HashMap>>> +) -> ConfigResult where T: Configuration, { let mut result = HashMap::new(); - let role_properties = parse_role_config(resource, role_name, &role.config, property_kinds); + let role_properties = parse_role_config(resource, role_name, &role.config, property_kinds)?; // for each role group ... for (role_group_name, role_group) in &role.role_groups { // ... compute the group properties ... let role_group_properties = - parse_role_config(resource, role_name, &role_group.config, property_kinds); + 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(); @@ -367,7 +374,7 @@ where result.insert(role_group_name.clone(), role_properties_copy); } - result + Ok(result) } /// Given a `config` object and the `property_kinds` vector, it uses the `Configuration::compute_*` methods @@ -385,7 +392,7 @@ fn parse_role_config( role_name: &str, config: &CommonConfiguration, property_kinds: &[PropertyNameKind], -) -> HashMap>> +) -> ConfigResult>>> where T: Configuration, { @@ -395,57 +402,58 @@ where match property_kind { PropertyNameKind::File(file) => result.insert( property_kind.clone(), - parse_file_properties(resource, role_name, config, file), + parse_file_properties(resource, role_name, config, file)?, ), PropertyNameKind::Env => result.insert( property_kind.clone(), - parse_env_properties(resource, role_name, config), + parse_env_properties(resource, role_name, config)?, ), PropertyNameKind::Cli => result.insert( property_kind.clone(), - parse_cli_properties(resource, role_name, config), + parse_cli_properties(resource, role_name, config)?, ), }; } - result + + Ok(result) } fn parse_cli_properties( resource: &::Configurable, role_name: &str, config: &CommonConfiguration, -) -> BTreeMap> +) -> ConfigResult>> 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).unwrap(); + 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())); } - final_properties + Ok(final_properties) } fn parse_env_properties( resource: &::Configurable, role_name: &str, config: &CommonConfiguration, -) -> BTreeMap> +) -> ConfigResult>> 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).unwrap(); + 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())); } - final_properties + Ok(final_properties) } fn parse_file_properties( @@ -453,15 +461,12 @@ fn parse_file_properties( role_name: &str, config: &CommonConfiguration, file: &str, -) -> BTreeMap> +) -> ConfigResult>> 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) - .unwrap(); + let mut final_properties = config.config.compute_files(resource, role_name, file)?; // ...followed by config_overrides from the role // For Conf files only process overrides that match our file name @@ -471,7 +476,7 @@ where } } - final_properties + Ok(final_properties) } #[cfg(test)] @@ -1016,7 +1021,8 @@ mod tests { let property_kinds = vec![PropertyNameKind::Env]; - let config = transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds); + let config = + transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds).unwrap(); assert_eq!(config, expected); } @@ -1071,7 +1077,8 @@ mod tests { PropertyNameKind::Cli, ]; - let config = transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds); + let config = + transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds).unwrap(); assert_eq!(config, expected); } @@ -1160,7 +1167,7 @@ mod tests { } }}; - let all_config = transform_all_roles_to_config(&String::new(), roles); + let all_config = transform_all_roles_to_config(&String::new(), roles).unwrap(); assert_eq!(all_config, expected); } @@ -1211,7 +1218,7 @@ mod tests { ), }; - let role_config = transform_all_roles_to_config(&String::new(), roles); + let role_config = transform_all_roles_to_config(&String::new(), roles).unwrap(); let config = &format!( " From feba96263c7b66cc678c16d7efb60381f15aa9f1 Mon Sep 17 00:00:00 2001 From: maltesander Date: Thu, 16 Dec 2021 10:45:22 +0100 Subject: [PATCH 4/4] Fixed breaking changelog from parse_* to transform_all_roles_to_config --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78388931f..d3bc8e15a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] ### Changed -- BREAKING: Introduced proper error handling for `parse_cli_properties`, `parse_env_properties` and `parse_file_properties` ([#282]). +- BREAKING: Introduced proper (Result) error handling for `transform_all_roles_to_config` ([#282]). - BREAKING: `Configuration::compute_*` are now invoked even when `config` field is not provided on `Role`/`RoleGroup` ([#282]). - `CommonConfiguration::config` is no longer `Option`al - `Role::config` is no longer `Option`al