fix(cfn-include): cfn-include fails in monocdk #11595

skinny85 · 2020-11-19T21:47:55Z

The cloudformation-include module generates a big JSON
file at build time that contains the mapping from the CloudFormation resource type
to the fully-qualified class name of the corresponding L1
(something like "AWS::S3::Bucket": "@aws-cdk/aws-s3.CfnBucket").
The problem is that mono-CDK re-packages all of the per-service
modules into one big module,
and requiring the module from the mapping fails for it.

Solve the issue by adding an additional build step in mono-CDK that re-writes that file with the mapping values changed
(to something like "monocdk/aws-s3.CfnBucket").

Fixes #11342

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

gitpod-io · 2020-11-19T21:48:03Z

skinny85 · 2020-11-19T21:51:15Z

I unfortunately only tested this change manually. I tried adding a simple unit test for cloudformation-include in the monocdk package, but unfortunately it passes even without any changes, as the @aws-cdk/aws-<service> modules are all present in the top-level node_modules 😕.

@nija-at let me know if this solution makes sense. If yes, we can also add it to the aws-cdk-lib package on the v2-main branch.

nija-at

We need to identify a way to test this. We can't afford for this to regress if we decide to change something in aws-cdk-lib and it breaks cfn-include again.

Can we add an integration test instead?

@nija-at let me know if this solution makes sense. If yes, we can also add it to the aws-cdk-lib package on the v2-main branch.

Add it into the aws-cdk-lib package in master and it will automatically get merged into v2-main

skinny85 · 2020-11-24T00:44:23Z

@nija-at after some digging and trail-and-error, I was able to add a unit test in monocdk that correctly checks this (I looked into an integration test, but it looked very painful).

Let me know if this solution works for you!

nija-at

Can we move this into a separate package under the tools/ folder so that it can be reused across monocdk and aws-cdk-lib?

ubergen might be a good spot but would be nice to have a separate one so it's kept separate and decoupled.

skinny85 · 2020-11-26T00:20:21Z

Can we move this into a separate package under the tools/ folder so that it can be reused across monocdk and aws-cdk-lib?

ubergen might be a good spot but would be nice to have a separate one so it's kept separate and decoupled.

@nija-at done! Please give the PR another look 🙂

packages/monocdk/package.json

packages/monocdk/test/cfn-include/cfn-include.test.ts

skinny85 · 2020-12-01T00:05:21Z

Keep the name terse. just call it cfninclude-rewriter.

This has been done. @nija-at please take another look!

nija-at

At this point, this discussion is likely more effective and faster to converge synchronously.

tools/cfninclude-rewriter/bin/cfninclude-rewriter.ts

packages/monocdk/test/cfn-include/cfn-include.test.ts

skinny85 · 2020-12-08T22:55:52Z

@nija-at I've re-structured the solution completely, by moving the logic of re-writing the file to ubergen, and the test from a unit test to an integ test. Please re-review!

nija-at

Much better set of changes than the previous iteration. Thanks for taking the time to fix this.

These comments are less intrusive.

packages/aws-cdk/test/integ/cli/monocdk.integtest.js

packages/aws-cdk/test/integ/cli/.gitignore

packages/aws-cdk/test/integ/cli/monocdk.integtest.js

The cloudformation-include module generates a big JSON file at build time that contains the mapping from the CloudFormation resource type to the fully-qualified class name of the corresponding L1 (something like "AWS::S3::Bucket": "@aws-cdk/aws-s3.CfnBucket"). The problem is that mono-CDK re-packages all of the per-service modules into one big module, and requiring the module from the mapping fails for it. Solve the issue by adding an additional build step in ubergen that re-writes that file with the mapping values changed (to something like "monocdk/aws-s3.CfnBucket"). Fixes aws#11342

nija-at

LGTM! Some minor comments below.

You will need to make changes to cdk-ops to add this integration test for monocdk to cdk-master and aws-cdk-lib to v2-main.

packages/aws-cdk/test/integ/cli/bootstrapping.integtest.ts

packages/aws-cdk/test/integ/common/jest-test.bash

packages/aws-cdk/test/integ/uberpackage/monocdk.integtest.ts

mergify · 2020-12-11T18:51:43Z

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 · 2020-12-11T19:20:39Z

AWS CodeBuild CI Report

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

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

mergify · 2020-12-11T19:21:51Z

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 changes from #11595 moved some helper TypeScript files around. When running without compiling first (like in the pipeline, or in the canaries), we need to install the dependencies those files need manually. Unfortunately, after the move, they are now installed in a sibling directory, not in any ancestor directory, and so `require`ing them fails.

…tor (#12029) The changes from #11595 moved some helper TypeScript files around. When running without compiling first (like in the pipeline, or in the canaries), we need to install the dependencies those files need manually. Unfortunately, after the move, they are now installed in a sibling directory, not in any ancestor directory, and so `require`ing them fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The cloudformation-include module generates a big JSON file at build time that contains the mapping from the CloudFormation resource type to the fully-qualified class name of the corresponding L1 (something like "AWS::S3::Bucket": "@aws-cdk/aws-s3.CfnBucket"). The problem is that mono-CDK re-packages all of the per-service modules into one big module, and requiring the module from the mapping fails for it. Solve the issue by adding an additional build step in mono-CDK that re-writes that file with the mapping values changed (to something like "monocdk/aws-s3.CfnBucket"). Fixes aws#11342 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…tor (aws#12029) The changes from aws#11595 moved some helper TypeScript files around. When running without compiling first (like in the pipeline, or in the canaries), we need to install the dependencies those files need manually. Unfortunately, after the move, they are now installed in a sibling directory, not in any ancestor directory, and so `require`ing them fails. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

When we did the changes to fix cloudformation-include in aws#11595, we did not account for the fact that the `@aws-cdk/core` is not mapped to `uberpackage/core`, but instead just to the `uberpackage` root namespace. Special-case the `@aws-cdk/core` module in ubergen when transforming the `cfn-types-2-classes.json` file.

When we did the changes to fix cloudformation-include in #11595, we did not account for the fact that the `@aws-cdk/core` is not mapped to `uberpackage/core`, but instead just to the `uberpackage` root namespace. Special-case the `@aws-cdk/core` module in ubergen when transforming the `cfn-types-2-classes.json` file. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…12758) When we did the changes to fix cloudformation-include in aws#11595, we did not account for the fact that the `@aws-cdk/core` is not mapped to `uberpackage/core`, but instead just to the `uberpackage` root namespace. Special-case the `@aws-cdk/core` module in ubergen when transforming the `cfn-types-2-classes.json` file. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 requested a review from nija-at November 19, 2020 21:47

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 19, 2020

skinny85 added the pr-linter/exempt-test The PR linter will not require test changes label Nov 19, 2020

SomayaB assigned skinny85 Nov 20, 2020

nija-at suggested changes Nov 20, 2020

View reviewed changes

nija-at changed the title ~~fix(cfn-include): cfn-include fails in mono-CDK~~ fix(cfn-include): cfn-include fails in monocdk Nov 20, 2020

skinny85 force-pushed the fix/cfn-include-monocdk branch from d7353d9 to fe4e907 Compare November 24, 2020 00:20

skinny85 removed the pr-linter/exempt-test The PR linter will not require test changes label Nov 24, 2020

nija-at suggested changes Nov 25, 2020

View reviewed changes

skinny85 force-pushed the fix/cfn-include-monocdk branch 2 times, most recently from 9da4ce9 to 99c4666 Compare November 26, 2020 00:19

skinny85 requested a review from nija-at November 26, 2020 00:20

nija-at suggested changes Nov 26, 2020

View reviewed changes

packages/monocdk/package.json Outdated Show resolved Hide resolved

packages/monocdk/test/cfn-include/cfn-include.test.ts Outdated Show resolved Hide resolved

skinny85 force-pushed the fix/cfn-include-monocdk branch from 99c4666 to ac3ed43 Compare December 1, 2020 00:04

skinny85 requested a review from nija-at December 1, 2020 00:04

nija-at reviewed Dec 1, 2020

View reviewed changes

tools/cfninclude-rewriter/bin/cfninclude-rewriter.ts Outdated Show resolved Hide resolved

packages/monocdk/test/cfn-include/cfn-include.test.ts Outdated Show resolved Hide resolved

skinny85 force-pushed the fix/cfn-include-monocdk branch from ac3ed43 to 1d60f92 Compare December 8, 2020 22:54

skinny85 requested a review from nija-at December 8, 2020 22:55

skinny85 force-pushed the fix/cfn-include-monocdk branch from 1d60f92 to ef41589 Compare December 8, 2020 22:59

nija-at suggested changes Dec 9, 2020

View reviewed changes

packages/aws-cdk/test/integ/cli/monocdk.integtest.js Outdated Show resolved Hide resolved

packages/aws-cdk/test/integ/cli/.gitignore Outdated Show resolved Hide resolved

packages/aws-cdk/test/integ/cli/monocdk.integtest.js Outdated Show resolved Hide resolved

skinny85 added 4 commits December 10, 2020 18:08

Basic test is working!

556ba78

Move the tests into a diffent directory.

11840b1

Move all shared infrastructure to a separate directory.

8c3ad08

skinny85 force-pushed the fix/cfn-include-monocdk branch from ef41589 to 8c3ad08 Compare December 11, 2020 02:20

nija-at added the pr/do-not-merge This PR should not be merged at this time. label Dec 11, 2020

nija-at approved these changes Dec 11, 2020

View reviewed changes

packages/aws-cdk/test/integ/cli/bootstrapping.integtest.ts Outdated Show resolved Hide resolved

packages/aws-cdk/test/integ/common/jest-test.bash Outdated Show resolved Hide resolved

packages/aws-cdk/test/integ/uberpackage/monocdk.integtest.ts Outdated Show resolved Hide resolved

skinny85 added 2 commits December 11, 2020 10:10

Implement changes from Niranajan's last comments.

0e53581

Fix the path to the app in the 'withCdkApp' helper.

4b813ed

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Dec 11, 2020

Merge branch 'master' into fix/cfn-include-monocdk

70e52a0

mergify bot merged commit 45e43f2 into aws:master Dec 11, 2020

skinny85 mentioned this pull request Dec 11, 2020

chore(cli): correctly install dependencies in integ tests after refactor #12029

Merged

skinny85 deleted the fix/cfn-include-monocdk branch December 11, 2020 23:36

skinny85 mentioned this pull request Jan 28, 2021

fix(cfn-include): AWS::CloudFormation resources fail in monocdk #12758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cfn-include): cfn-include fails in monocdk #11595

fix(cfn-include): cfn-include fails in monocdk #11595

skinny85 commented Nov 19, 2020

gitpod-io bot commented Nov 19, 2020 •

edited

Loading

skinny85 commented Nov 19, 2020

nija-at left a comment •

edited

Loading

skinny85 commented Nov 24, 2020

nija-at left a comment

skinny85 commented Nov 26, 2020

skinny85 commented Dec 1, 2020

nija-at left a comment

skinny85 commented Dec 8, 2020

nija-at left a comment •

edited

Loading

nija-at left a comment

mergify bot commented Dec 11, 2020

aws-cdk-automation commented Dec 11, 2020

mergify bot commented Dec 11, 2020

fix(cfn-include): cfn-include fails in monocdk #11595

fix(cfn-include): cfn-include fails in monocdk #11595

Conversation

skinny85 commented Nov 19, 2020

gitpod-io bot commented Nov 19, 2020 • edited Loading

skinny85 commented Nov 19, 2020

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

skinny85 commented Nov 24, 2020

nija-at left a comment

Choose a reason for hiding this comment

skinny85 commented Nov 26, 2020

skinny85 commented Dec 1, 2020

nija-at left a comment

Choose a reason for hiding this comment

skinny85 commented Dec 8, 2020

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 11, 2020

aws-cdk-automation commented Dec 11, 2020

AWS CodeBuild CI Report

mergify bot commented Dec 11, 2020

gitpod-io bot commented Nov 19, 2020 •

edited

Loading

nija-at left a comment •

edited

Loading

nija-at left a comment •

edited

Loading