fix(cfn-include): fix issues in Conditions handling #9142
Conversation
| throw new Error(`Output with name '${logicalId}' refers to a Condition with name\ | ||
| '${outputAttributes.Condition}' which was not found in this template`); | ||
| throw new Error(`Output with name '${logicalId}' refers to a Condition with name '${outputAttributes.Condition}' which was not found in this template`); |
There was a problem hiding this comment.
why remove the line break here?
There was a problem hiding this comment.
Because the unindented code on line 266 screws up the vertical guides in my IDE. I'll do it in a different way though.
| @@ -251,6 +251,41 @@ describe('CDK Include', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| test('correctly change references to Conditions when renaming them', () => { | |||
There was a problem hiding this comment.
I like the handling of two cases with one test here, nicely done.
| } | ||
|
|
||
| const self = this; | ||
| const cfnParser = new cfn_parse.CfnParser({ | ||
| finder: { | ||
| findResource() { throw new Error('Using GetAtt in Condition definitions is not allowed'); }, |
There was a problem hiding this comment.
We still don't have a unit test that hits this case
There was a problem hiding this comment.
You're right. Will add.
| const cfnCondition = new core.CfnCondition(this.conditionsScope, conditionName, { | ||
| expression: cfnParser.parseValue(this.template.Conditions[conditionName]), | ||
| }); | ||
|
|
||
| // ToDo handle renaming of the logical IDs of the conditions |
There was a problem hiding this comment.
Don't think we still need this ToDo
There was a problem hiding this comment.
It's basically for checking the value of this.preserveLogicalIds, which today is always true. Just a reminder to change here once that's no longer the case 🙂.
The current Conditions handling in cloudformation-include has a few flaws, that are all corrected in this change: - Do not fail when a Condition and a Resource share the same logical ID - Handle changing the logical IDs of Conditions used in Fn::If expressions - Handle changing the logical IDs of Conditions referenced inside the Conditions section - Do not fail when referring to a Parameter from inside a Condition
skinny85
force-pushed
the
fix/cfn-include-conditions
branch
from
27a9961 to
aa62448
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The current Conditions handling in cloudformation-include has a few flaws, that are all corrected in this change: - Do not fail when a Condition and a Resource share the same logical ID - Handle changing the logical IDs of Conditions used in Fn::If expressions - Handle changing the logical IDs of Conditions referenced inside the Conditions section - Do not fail when referring to a Parameter from inside a Condition ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
skinny85
added this to the "Migrating from CloudFormation to CDK" - Developer Preview milestone
The current Conditions handling in cloudformation-include has a few flaws,
that are all corrected in this change:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license