fix(cloudwatch): skip MathExpression validation when prop is a token #36487
Conversation
gasolima
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-maintainer-review
| }); | ||
| console.log({ resources: JSON.stringify(Template.fromStack(stack).findResources('AWS::CloudWatch::Alarm'), null, 2) }); | ||
|
|
||
| Template.fromStack(stack).hasResourceProperties('AWS::CloudWatch::Alarm', { |
There was a problem hiding this comment.
A quick question : Does the Lazy variable already resolve on cdk synth ? (This test is checking cdk synth generated templates)
There was a problem hiding this comment.
Tokens resolve at deploy time => We should add a test to cover these conditions i.e the template should have the values in [Token* format
There was a problem hiding this comment.
Does the Lazy variable already resolve on cdk synth ? (This test is checking cdk synth generated templates)
It does
Tokens resolve at deploy time => We should add a test to cover these conditions i.e the template should have the values in [Token* format
The code add for checking if it's token or not happening before the synth time, so for the checking part it doesn't make difference if it's going to resolve in synth time or deploy time
There was a problem hiding this comment.
For checking part it doesnot BUT we are checking for the value token here which will not be correct if I use tokens that resolve at deploy time.
There was a problem hiding this comment.
the checks run before the resolve so this test fails, if i removed the fix implementation
There was a problem hiding this comment.
Since the deploy and synth time resolution both will be caught here, adding extra checks doesnot make sense as synth time resolution will always be checked in the condition added above
gasolima
force-pushed
the
fix-validation-error
branch
from
106aafd to
b888210
Compare
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status✅ The pull request has been merged at b888210 This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
|
Comments on closed issues and PRs are hard for our team to see. |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
3 participants
Issue # (if applicable)
Closes #.
Reason for this change
MathExpressionclass has multiple props that needs to be alphanumeric, that check for this is already there, but the problem is the checker doesn't check first if it's token or not. so for any tokens it always throw an error since it has$even if the resolve of this token is alphanumeric string.Description of changes
I'm checking if it's token or not, and if it's i'm passing the alphanumeric checker, since in this case u can't check it.
Describe any new or updated permissions being added
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license