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

fix: removes preparepp() cdkV1 shim layer #11313

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

akshbhu
Copy link
Contributor

@akshbhu akshbhu commented Nov 2, 2022

Description of changes

For Context :

We removed the prepareApp implementation exported in cdk V1 and replaced with ShimImplementation : https://github.com/aws-amplify/amplify-cli/blob/cdkv2/packages/amplify-category-geo/src/service-utils/prepare-app.ts required to generate same cfn.

The fix is inspired by https://github.com/aws/aws-cdk/blob/bd056d1d38a2d3f43efe4f857c4d38b30fb9b681/packages/%40aws-cdk/assertions/lib/template.ts#L298-L310 .
This performs deep evaluation of the stack and fills missing dependsOn Custom lambda resources.

There are two changes in PR

  1. Synthesizing cfn template using root.synth() which generates full cloud assembly with dependencies. Similar behaviour is not achieved by _.toCloudformation() w/o prepareApp().
  2. Calling root.synth() in cdkV2 uses defaultStackSynthesizer by default which was creating default cfn parameters for bootstartping cdk project. This is changed to previous behaviour of using lagacyStacksyntheiser

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Pull request labels are added

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

@akshbhu akshbhu requested a review from a team as a code owner November 2, 2022 23:24
sobolk
sobolk previously approved these changes Nov 2, 2022
jhockett
jhockett previously approved these changes Nov 2, 2022
return assembly.getStackArtifact(this.artifactId).template;
}
// if this is a nested stack ( i.e. it has a parent), then just read the template as a string
return JSON.parse(fs.readFileSync(path.join(assembly.directory, this.templateFile)).toString('utf-8'));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer this broken up for readability

Suggested change
return JSON.parse(fs.readFileSync(path.join(assembly.directory, this.templateFile)).toString('utf-8'));
const template = fs.readFileSync(path.join(assembly.directory, this.templateFile);
return JSON.parse(template).toString('utf-8'));

@akshbhu akshbhu dismissed stale reviews from jhockett and sobolk via 01e1d2b November 3, 2022 16:49
@akshbhu akshbhu merged commit e3908c8 into aws-amplify:cdkv2 Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants