Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cfn-include): support logical id overrides #8529

Merged
merged 5 commits into from
Jun 24, 2020

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Jun 13, 2020

Previously, we created the Tokens needed for references like Ref and Fn::GetAtt
without actually referencing the created L1s in the template,
just by their logical IDs.
That's not strictly correct,
because users of CfnInclude can call overrideLogicalId()
on the CloudFormation elements retrieved from the template,
and we need to make sure to also reflect that in all references to that resource.

Change the code converting from CloudFormation to CDK values to actually retrieve
the appropriate L1 objects when encountering expressions like Ref and Fn::GetAtt.
This also gives us correctly working cross-stack references for free.

Related to #7375


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@skinny85 skinny85 requested a review from nija-at June 13, 2020 00:30
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 13, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: abeafbe
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@skinny85 skinny85 force-pushed the feat/cfn-include-cross-stack-refs branch from abeafbe to 287d48f Compare June 23, 2020 00:57
@skinny85 skinny85 changed the title chore(cfn-include): add a test for cross-stack references working as expected feat(cfn-include): handle re-naming the logical IDs of elements in the template Jun 23, 2020
@skinny85 skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Jun 23, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 287d48f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@rix0rrr, @eladb - I'm not familiar with the existing patterns in core. Can you take a look to see if the design fits as expected?

packages/@aws-cdk/core/lib/cfn-parse.ts Outdated Show resolved Hide resolved
@nija-at nija-at requested review from eladb and rix0rrr June 23, 2020 09:44
@@ -110,11 +122,11 @@ export class FromCloudFormation {
return ret;
}

public static parseCreationPolicy(policy: any): CfnCreationPolicy | undefined {
public static parseCreationPolicy(policy: any, options: ParseCfnOptions): CfnCreationPolicy | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing around all the options everywhere feels a little dirty to me.

I'd think I'd prefer a class CfnParser or something which has the resolver as a member.

Not going to force you to change it, but I have a feeling it will be cleaner. Give it a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. @rix0rrr let me know if you like this way better.

…e template

Previously, we created the Tokens needed for references like Ref and Fn::GetAtt
without actually referencing the created L1s in the template,
just by their logical IDs.
That's not strictly correct,
because users of CfnInclude can call overrideLogicalId()
on the CloudFormation elements retrieved from the template,
and we need to make sure to also reflect that in all references to that resource.

Change the code converting from CloudFormation to CDK values to actually retrieve
the appropriate L1 objects when encountering expressions like Ref and Fn::GetAtt.
This also gives us correctly working cross-stack references for free.

Related to aws#7375
@skinny85 skinny85 force-pushed the feat/cfn-include-cross-stack-refs branch from 287d48f to cb57a09 Compare June 24, 2020 00:17
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: cb57a09
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 462dcd6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nija-at nija-at changed the title feat(cfn-include): handle re-naming the logical IDs of elements in the template feat(cfn-include): support logical id overrides Jun 24, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2020

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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7ccb2b9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2020

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).

@mergify mergify bot merged commit d9c4f5e into aws:master Jun 24, 2020
@skinny85 skinny85 deleted the feat/cfn-include-cross-stack-refs branch June 24, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants