search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.51k stars 3.85k forks source link

cdk.Fn: improve conditionEquals parameters types #30036

Open AllanOricil opened 4 months ago

AllanOricil commented 4 months ago

Describe the feature

Because cdk.Fn.conditionEquals is expecting 2 parameters of type any

image

people can logically think that the following condition, which compares "string" with "string", should work

cdk.Fn.conditionEquals(props.createBucketParameter.valueAsString, "true"),

However, it does not. The proper way to configure this condition is to pass CfnParameter reference as follows, 

cdk.Fn.conditionEquals(props.createBucketParameter, "true"),

which leads to this template piece:

image

At first, this behavior seems illogical, because:

  1. when reading a code that isn't using asValueString, the comparison is clearly between "CfnParameter" and "string" types, which, because of the lack of proper type definition for parameters, won't be considered as an option

  2. the types in the conditionEquals method are of "any", which can make people think that they must compare similar types, like string and string, boolean and boolean, or even CfnParameter to CfnParameter. However, as shown previously, when using the type CfnParameters, the first argument must be its reference and not itsvalueAsString.

Use Case

Developers will clearly know what types can be compared in cdk.Fn.conditionEquals when writting or reading code, which improves DX, and therefore efficiency.

Proposed Solution

Add all type permutations that are possible for those 2 parameters in cdk.Fn.conditionEquals

Other Information

No response

Acknowledgements

CDK version used

2.138

Environment details (OS name and version, etc.)

macos

khushail commented 4 months ago

Hey @AllanOricil , thanks for reporting this. From the Read me of conditionequals , it is apparent as what you are saying is true.

https://github.com/aws/aws-cdk/blob/90f1aa9f1c9b1906953209b032e999fe4fe52bd4/packages/aws-cdk-lib/README.md?plain=1#L994

it seems confusing to look at this code inline documentation though as mentioned by you- https://github.com/aws/aws-cdk/blob/5dd72b89f20b1246ad125440e42449acd80c8be7/packages/aws-cdk-lib/core/lib/cfn-fn.ts#L293

I can see this PR was submitted to bring Cloudformation parameter constraint only and it would be helpful to have the clear docs to understand the functionality. Please feel free to submit a PR.

Since this might be a breaking change, I am adding the required labels for Team's input here.