From 8d9e7703e96ee5ca528c007ff2f37c52add11ee3 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Feb 2024 10:37:40 +1300 Subject: [PATCH 1/3] Move bucket name logic to `S3BucketState::bucket_name` and reference it in usages. --- examples/envman/src/cmds/app_upload_cmd.rs | 8 +------- examples/envman/src/flows/app_upload_flow.rs | 8 +------- examples/envman/src/flows/env_deploy_flow.rs | 4 ++-- .../items/peace_aws_s3_bucket/s3_bucket_state.rs | 13 +++++++++++++ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/examples/envman/src/cmds/app_upload_cmd.rs b/examples/envman/src/cmds/app_upload_cmd.rs index 3b8aa1aad..0e151406d 100644 --- a/examples/envman/src/cmds/app_upload_cmd.rs +++ b/examples/envman/src/cmds/app_upload_cmd.rs @@ -54,13 +54,7 @@ impl AppUploadCmd { let profile_key = WorkspaceParamsKey::Profile; let s3_object_params_spec = S3ObjectParams::::field_wise_spec() - .with_bucket_name_from_map(|s3_bucket_state: &S3BucketState| match s3_bucket_state { - S3BucketState::None => None, - S3BucketState::Some { - name, - creation_date: _, - } => Some(name.clone()), - }) + .with_bucket_name_from_map(S3BucketState::bucket_name) .build(); let mut cmd_ctx = { diff --git a/examples/envman/src/flows/app_upload_flow.rs b/examples/envman/src/flows/app_upload_flow.rs index ac0b15623..3f5b560b5 100644 --- a/examples/envman/src/flows/app_upload_flow.rs +++ b/examples/envman/src/flows/app_upload_flow.rs @@ -98,13 +98,7 @@ impl AppUploadFlow { let s3_object_params_spec = S3ObjectParams::::field_wise_spec() .with_file_path(web_app_path_local) .with_object_key(object_key) - .with_bucket_name_from_map(|s3_bucket_state: &S3BucketState| match s3_bucket_state { - S3BucketState::None => None, - S3BucketState::Some { - name, - creation_date: _, - } => Some(name.clone()), - }) + .with_bucket_name_from_map(S3BucketState::bucket_name) .build(); Ok(AppUploadFlowParamsSpecs { diff --git a/examples/envman/src/flows/env_deploy_flow.rs b/examples/envman/src/flows/env_deploy_flow.rs index 3853b02b4..2d8d35d17 100644 --- a/examples/envman/src/flows/env_deploy_flow.rs +++ b/examples/envman/src/flows/env_deploy_flow.rs @@ -17,7 +17,7 @@ use crate::{ peace_aws_iam_policy::{IamPolicyItem, IamPolicyParams, IamPolicyState}, peace_aws_iam_role::{IamRoleItem, IamRoleParams}, peace_aws_instance_profile::{InstanceProfileItem, InstanceProfileParams}, - peace_aws_s3_bucket::{S3BucketItem, S3BucketParams}, + peace_aws_s3_bucket::{S3BucketItem, S3BucketParams, S3BucketState}, peace_aws_s3_object::{S3ObjectItem, S3ObjectParams}, }, model::{EnvManError, RepoSlug, WebApp}, @@ -161,7 +161,7 @@ impl EnvDeployFlow { .build(); let s3_object_params_spec = S3ObjectParams::::field_wise_spec() .with_file_path(web_app_path_local) - .with_bucket_name(bucket_name) + .with_bucket_name_from_map(S3BucketState::bucket_name) .with_object_key(object_key) .build(); diff --git a/examples/envman/src/items/peace_aws_s3_bucket/s3_bucket_state.rs b/examples/envman/src/items/peace_aws_s3_bucket/s3_bucket_state.rs index 4be25c2ed..46976f843 100644 --- a/examples/envman/src/items/peace_aws_s3_bucket/s3_bucket_state.rs +++ b/examples/envman/src/items/peace_aws_s3_bucket/s3_bucket_state.rs @@ -22,6 +22,19 @@ pub enum S3BucketState { }, } +impl S3BucketState { + /// Returns the bucket name if the bucket exists. + pub fn bucket_name(&self) -> Option { + match self { + S3BucketState::None => None, + S3BucketState::Some { + name, + creation_date: _, + } => Some(name.clone()), + } + } +} + impl fmt::Display for S3BucketState { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { From 616694e41911a664e70922a04fe51f1ce0851835 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Feb 2024 12:02:08 +1300 Subject: [PATCH 2/3] Move `iam_policy_arn_version` logic to `IamPolicyState`. --- examples/envman/src/cmds/env_cmd.rs | 13 ++----------- examples/envman/src/flows/env_deploy_flow.rs | 14 ++------------ .../peace_aws_iam_policy/iam_policy_state.rs | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/examples/envman/src/cmds/env_cmd.rs b/examples/envman/src/cmds/env_cmd.rs index b01d082f2..912271c15 100644 --- a/examples/envman/src/cmds/env_cmd.rs +++ b/examples/envman/src/cmds/env_cmd.rs @@ -1,6 +1,6 @@ use futures::future::LocalBoxFuture; use peace::{ - cfg::{app_name, item_id, state::Generated, Profile}, + cfg::{app_name, item_id, Profile}, cmd::{ ctx::CmdCtx, scopes::{ @@ -57,16 +57,7 @@ impl EnvCmd { let iam_role_params_spec = IamRoleParams::::field_wise_spec() .with_name_from_map(|profile: &Profile| Some(profile.to_string())) .with_path(iam_role_path) - .with_managed_policy_arn_from_map(|iam_policy_state: &IamPolicyState| { - let IamPolicyState::Some { - policy_id_arn_version: Generated::Value(policy_id_arn_version), - .. - } = iam_policy_state - else { - return None; - }; - Some(policy_id_arn_version.arn().to_string()) - }) + .with_managed_policy_arn_from_map(IamPolicyState::policy_id_arn_version) .build(); let CmdOpts { profile_print } = cmd_opts; diff --git a/examples/envman/src/flows/env_deploy_flow.rs b/examples/envman/src/flows/env_deploy_flow.rs index 2d8d35d17..9e53e3fd8 100644 --- a/examples/envman/src/flows/env_deploy_flow.rs +++ b/examples/envman/src/flows/env_deploy_flow.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use peace::{ - cfg::{app_name, flow_id, item_id, state::Generated, Profile}, + cfg::{app_name, flow_id, item_id, Profile}, params::{Params, ParamsSpec}, rt_model::{Flow, ItemGraphBuilder}, }; @@ -134,17 +134,7 @@ impl EnvDeployFlow { let iam_role_params_spec = IamRoleParams::::field_wise_spec() .with_name(iam_role_name) .with_path(path.clone()) - .with_managed_policy_arn_from_map(|iam_policy_state: &IamPolicyState| { - if let IamPolicyState::Some { - policy_id_arn_version: Generated::Value(policy_id_arn_version), - .. - } = iam_policy_state - { - Some(policy_id_arn_version.arn().to_string()) - } else { - None - } - }) + .with_managed_policy_arn_from_map(IamPolicyState::policy_id_arn_version) .build(); let instance_profile_params_spec = InstanceProfileParams::::field_wise_spec() .with_name(instance_profile_name) diff --git a/examples/envman/src/items/peace_aws_iam_policy/iam_policy_state.rs b/examples/envman/src/items/peace_aws_iam_policy/iam_policy_state.rs index 3e0e76280..99a60d5aa 100644 --- a/examples/envman/src/items/peace_aws_iam_policy/iam_policy_state.rs +++ b/examples/envman/src/items/peace_aws_iam_policy/iam_policy_state.rs @@ -32,6 +32,21 @@ pub enum IamPolicyState { }, } +impl IamPolicyState { + /// Returns the `policy_id_arn_version` if it exists. + pub fn policy_id_arn_version(&self) -> Option { + if let IamPolicyState::Some { + policy_id_arn_version: Generated::Value(policy_id_arn_version), + .. + } = self + { + Some(policy_id_arn_version.arn().to_string()) + } else { + None + } + } +} + fn path_default() -> String { String::from("/") } From f11d1995e4deb5feb1896569aeac8c684f9a32ec Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Feb 2024 13:29:30 +1300 Subject: [PATCH 3/3] Add `mapping_functions.md` describing the params specs recall options. --- doc/src/SUMMARY.md | 3 +- .../item/item_parameters/mapping_functions.md | 133 ++++++++++++++++++ ..._definition.md => params_specification.md} | 20 ++- 3 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 doc/src/technical_concepts/item/item_parameters/mapping_functions.md rename doc/src/technical_concepts/item/item_parameters/{params_definition.md => params_specification.md} (89%) diff --git a/doc/src/SUMMARY.md b/doc/src/SUMMARY.md index ecf0841ad..bd259e2bc 100644 --- a/doc/src/SUMMARY.md +++ b/doc/src/SUMMARY.md @@ -20,8 +20,9 @@ - [Item](technical_concepts/item.md) - [Item Parameters](technical_concepts/item/item_parameters.md) - [Value Specification](technical_concepts/item/item_parameters/value_specification.md) - - [Params Definition](technical_concepts/item/item_parameters/params_definition.md) + - [Params Specification](technical_concepts/item/item_parameters/params_specification.md) - [Params Framework Support](technical_concepts/item/item_parameters/params_framework_support.md) + - [Mapping Functions](technical_concepts/item/item_parameters/mapping_functions.md) - [Item Graph](technical_concepts/item_graph.md) - [Initialization](technical_concepts/item_graph/initialization.md) - [State Inspection](technical_concepts/item_graph/state_inspection.md) diff --git a/doc/src/technical_concepts/item/item_parameters/mapping_functions.md b/doc/src/technical_concepts/item/item_parameters/mapping_functions.md new file mode 100644 index 000000000..3948365cf --- /dev/null +++ b/doc/src/technical_concepts/item/item_parameters/mapping_functions.md @@ -0,0 +1,133 @@ +# Mapping Functions + +> Github Issue: [#156][#156] + +Mapping functions could be passed to a `Params`'s `FieldWiseSpec`. There are implications whether or not it is implemented. + + +[#156]: https://github.com/azriel91/peace/issues/156 + + +### Summary + +| With it | Without it | +|:-----------------------------------------|:-----------------------------------------------| +| Runtime error on mismatched types | Compilation error on mismatched types | +| Need to define mapping function key enum | Don't need to define mapping function key enum | +| Don't need to specify params spec for "used what was set last time" for mapping functions -- always set in cmd ctx | May forget specifying "used what was set last time" for mapping functions, and hit runtime error | + + +### With It + +### Developers + +1. Define an enum to name the function keys: + + ```rust + enum MappingFunctions { + BucketNameFromBucketState, + IamPolicyArnFromIamPolicyState, + } + ``` + +2. Define the mappings: + + ```rust + let mapping_functions = { + let mut mapping_functions = MappingFunctions::new(); + mapping_functions.insert(BucketNameFromBucketState, S3BucketState::bucket_name); + // .. + + mapping_functions + }; + ``` + + **Note:** + + If we want to have a compilation error here, the `MappingFunctions::insert` function needs to have a type parameter that tells it the `Item > Params > Field` that the mapping function is for. + + However, developers use `#[derive(Params)]` to derive the `FieldWiseSpec`, and requiring them to specify something like the following is arduous: + + ```rust + MappingFunction::insert(BucketNameFromBucketState, S3BucketState::bucket_name) + ``` + + +3. Pass `MappingFunctions` to `CmdCtxBuilder`, for each code instantiation (may be just one): + + ```rust + cmd_ctx_builder.with_mapping_functions(mapping_functions); + ``` + +4. Not have to call `.with_item_params::(..)` in subsequent calls. + + +#### Users + +1. Get runtime error if the mapping function type doesn't match, but it should be caught by tests. + + +#### Framework Maintainers + +1. `MappingFunctions` map will have magic logic to store the function argument types and return type. +2. Error reporting when types are mismatched. + + +### Without It + +#### Developers + +1. Define the item params spec: + + ```rust + // First execution + let s3_object_params_spec = S3ObjectParams::::field_wise_spec() + .with_file_path(web_app_path_local) + .with_object_key(object_key) + .with_bucket_name_from_map(S3BucketState::bucket_name) + .build(); + + // Subsequent executions + let s3_object_params_spec = S3ObjectParams::::field_wise_spec() + .with_bucket_name_from_map(S3BucketState::bucket_name) + .build(); + ``` + +2. Pass the item params spec to `CmdCtxBuilder`, for every separate code instantiation: + + ```rust + cmd_ctx_builder + .with_item_params::>( + item_id!("s3_object"), + s3_object_params_spec, + ) + ``` + + This is somewhat of an inconvenience, because if this isn't done, the user / developer will have a runtime error, which looks like this: + + ```md + peace_rt_model::params_specs_mismatch + + × Item params specs do not match with the items in the flow. + help: The following items either have not had a params spec provided previously, + or had contained a mapping function, which cannot be loaded from disk. + + So the params spec needs to be provided to the command context for: + + * s3_object + ``` + +When the closure passed to `with_*_from_map` doesn't have the argument type specified, or mismatches, the compilation error is still unclear. [rust#119888][rust#119888] will allow us to return a useful compilation error. + + +[rust#119888]: https://github.com/rust-lang/rust/pull/119888 + + +#### Users + +No runtime error, because it will be caught at compile time. + + +#### Framework Maintainers + +1. Error messages / diagnostics showing which `CmdCtx` is missing which item spec for which field, should be made clear. diff --git a/doc/src/technical_concepts/item/item_parameters/params_definition.md b/doc/src/technical_concepts/item/item_parameters/params_specification.md similarity index 89% rename from doc/src/technical_concepts/item/item_parameters/params_definition.md rename to doc/src/technical_concepts/item/item_parameters/params_specification.md index 3db532a70..09236fac0 100644 --- a/doc/src/technical_concepts/item/item_parameters/params_definition.md +++ b/doc/src/technical_concepts/item/item_parameters/params_specification.md @@ -1,4 +1,4 @@ -# Params Definition +# Params Specification For an item to work with different values, the values must be passed in. @@ -217,3 +217,21 @@ See: * [`optfield`](https://github.com/roignpar/optfield) * [`partial_derive`](https://github.com/rise0chen/partial_derive) * [`optional-field`](https://github.com/cvpartner/optional-field) + +## Implementation + +See the `params_derive` crate for code gen. + +The following command helps to see what's been generated. + +```bash +cargo expand --package peace_item_blank blank_params \ + | sd -f cm '^ #\[doc\(hidden\)\][\n](^[ ]{4}[a-z# ].+[\n])+^[ ]{4}\};\n' '' \ + | sd -f cm '^ #\[automatically_derived\][\n](^[ ]{4}[# a-z{].*[\n])+^[ ]{4}\{?\}\n' '' \ + | sd -f cm '^ #\[allow\(unused_qualifications\)\][\n](^[ ]{4}[# a-z{].*[\n])+^[ ]{4}\}\n' '' \ + | sd -f cm '^ #\[serde\(bound = ""\)\]' ' #[derive(Serialize, Deserialize)]\n #[serde(bound = "")]' \ + | sd -f cm '^ extern crate test;[\n](^[ ]{4}.*[\n])+^\}' '}' \ + | sd -f cm '^( pub struct [A-Za-z0-9_]+Partial)' ' #[derive(PartialEq, Eq)]\n$1' \ + | sd -f cm '^( #\[derivative\()' ' #[derive(derivative::Derivative)]\n$1' \ + | xclip -se c +```