Skip to content

Commit

Permalink
fix(cfn-include): cfn-include fails in monocdk (aws#11595)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
skinny85 authored and flochaz committed Jan 5, 2021
1 parent 7e95dce commit d40ccda
Show file tree
Hide file tree
Showing 22 changed files with 173 additions and 32 deletions.
2 changes: 1 addition & 1 deletion packages/aws-cdk/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ Following are the steps involved in running these tests:
- Download the previous version tarball from npm and extract the integration tests.
- Export a `FRAMWORK_VERSION` env variable based on the caller, and execute the integration tests of the previous version.

7. Our integration tests now run and have knowledge of which framework version they should [install](./test/integ/cli/cdk-helpers.ts#L74).
7. Our integration tests now run and have knowledge of which framework version they should [install](./test/integ/helpers/cdk.ts#L74).

That "basically" it, hope it makes sense...

Expand Down
6 changes: 3 additions & 3 deletions packages/aws-cdk/test/integ/cli/bootstrapping.integtest.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as fs from 'fs';
import * as path from 'path';
import { randomString, withDefaultFixture } from './cdk-helpers';
import { integTest } from './test-helpers';
import { randomString, withDefaultFixture } from '../helpers/cdk';
import { integTest } from '../helpers/test-helpers';

jest.setTimeout(600_000);

Expand Down Expand Up @@ -242,4 +242,4 @@ integTest('add tags, left alone on re-bootstrap', withDefaultFixture(async (fixt
expect(response.Stacks?.[0].Tags).toEqual([
{ Key: 'Foo', Value: 'Bar' },
]);
}));
}));
6 changes: 3 additions & 3 deletions packages/aws-cdk/test/integ/cli/cli.integtest.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { promises as fs } from 'fs';
import * as os from 'os';
import * as path from 'path';
import { retry, sleep } from './aws-helpers';
import { cloneDirectory, shell, withDefaultFixture } from './cdk-helpers';
import { integTest } from './test-helpers';
import { retry, sleep } from '../helpers/aws';
import { cloneDirectory, shell, withDefaultFixture } from '../helpers/cdk';
import { integTest } from '../helpers/test-helpers';

jest.setTimeout(600 * 1000);

Expand Down
15 changes: 2 additions & 13 deletions packages/aws-cdk/test/integ/cli/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,5 @@ echo '~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~'

cd $scriptdir

# Install these dependencies that the tests (written in Jest) need.
# Only if we're not running from the repo, because if we are the
# dependencies have already been installed by the containing 'aws-cdk' package's
# package.json.
if ! npx --no-install jest --version; then
echo 'Looks like we need to install jest first. Hold on.' >& 2
npm install --prefix . jest jest-junit aws-sdk
fi

# This must --runInBand because parallelism is arranged for inside the tests
# themselves and they must run in the same process in order to coordinate to
# make sure no 2 tests use the same region at the same time.
npx jest --runInBand --verbose "$@"
source ../common/jest-test.bash
invokeJest "$@"
15 changes: 15 additions & 0 deletions packages/aws-cdk/test/integ/common/jest-test.bash
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
function invokeJest() {
# Install these dependencies that the tests (written in Jest) need.
# Only if we're not running from the repo, because if we are the
# dependencies have already been installed by the containing 'aws-cdk' package's
# package.json.
if ! npx --no-install jest --version; then
echo 'Looks like we need to install jest first. Hold on.' >& 2
npm install --prefix . jest jest-junit aws-sdk
fi

# This must --runInBand because parallelism is arranged for inside the tests
# themselves and they must run in the same process in order to coordinate to
# make sure no 2 tests use the same region at the same time.
npx jest --runInBand --verbose "$@"
}
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as child_process from 'child_process';
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';
import { outputFromStack, AwsClients } from './aws-helpers';
import { outputFromStack, AwsClients } from './aws';
import { ResourcePool } from './resource-pool';
import { TestContext } from './test-helpers';

Expand Down Expand Up @@ -52,7 +52,7 @@ export function withCdkApp<A extends TestContext & AwsContext>(block: (context:
context.output.write(` Test directory: ${integTestDir}\n`);
context.output.write(` Region: ${context.aws.region}\n`);

await cloneDirectory(path.join(__dirname, 'app'), integTestDir, context.output);
await cloneDirectory(path.join(__dirname, '..', 'cli', 'app'), integTestDir, context.output);
const fixture = new TestFixture(
integTestDir,
stackNamePrefix,
Expand Down Expand Up @@ -92,6 +92,51 @@ export function withCdkApp<A extends TestContext & AwsContext>(block: (context:
};
}

export function withMonolithicCfnIncludeCdkApp<A extends TestContext>(block: (context: TestFixture) => Promise<void>) {
return async (context: A) => {
const uberPackage = process.env.UBERPACKAGE;
if (!uberPackage) {
throw new Error('The UBERPACKAGE environment variable is required for running this test!');
}

const randy = randomString();
const stackNamePrefix = `cdk-uber-cfn-include-${randy}`;
const integTestDir = path.join(os.tmpdir(), `cdk-uber-cfn-include-${randy}`);

context.output.write(` Stack prefix: ${stackNamePrefix}\n`);
context.output.write(` Test directory: ${integTestDir}\n`);

const awsClients = await AwsClients.default(context.output);
await cloneDirectory(path.join(__dirname, '..', 'uberpackage', 'cfn-include-app'), integTestDir, context.output);
const fixture = new TestFixture(
integTestDir,
stackNamePrefix,
context.output,
awsClients,
);

let success = true;
try {
let module = uberPackage;
if (FRAMEWORK_VERSION) {
module = `${module}@${FRAMEWORK_VERSION}`;
}
await fixture.shell(['npm', 'install', 'constructs', module]);

await block(fixture);
} catch (e) {
success = false;
throw e;
} finally {
if (process.env.INTEG_NO_CLEAN) {
process.stderr.write(`Left test directory in '${integTestDir}' ($INTEG_NO_CLEAN)\n`);
} else {
await fixture.dispose(success);
}
}
};
}

/**
* Default test fixture for most (all?) integ tests
*
Expand Down Expand Up @@ -370,10 +415,11 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
child.once('error', reject);

child.once('close', code => {
const output = (Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim();
if (code === 0 || options.allowErrExit) {
resolve((Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim());
resolve(output);
} else {
reject(new Error(`'${command.join(' ')}' exited with error code ${code}`));
reject(new Error(`'${command.join(' ')}' exited with error code ${code}. Output: \n${output}`));
}
});
});
Expand Down
File renamed without changes.
4 changes: 3 additions & 1 deletion packages/aws-cdk/test/integ/run-against-dist.bash
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ npmws=/tmp/cdk-rundist
rm -rf $npmws
mkdir -p $npmws

set -x

# This script must create 1 or 2 traps, and the 'trap' command will replace
# the previous trap, so get some 'dynamic traps' mechanism in place
TRAPS=()
Expand Down Expand Up @@ -137,4 +139,4 @@ function prepare_python_packages() {

function pip() {
pip_ "$@"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
!cfn-include-app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"app": "node cfn-include-app.js",
"versionReporting": false
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const path = require('path');

const uberPackage = process.env.UBERPACKAGE;
if (!uberPackage) {
throw new Error('The UBERPACKAGE environment variable is required for running this app!');
}

const cfn_inc = require(`${uberPackage}/cloudformation-include`);
const core = require(`${uberPackage}`);

const app = new core.App();
const stack = new core.Stack(app, 'Stack');
const cfnInclude = new cfn_inc.CfnInclude(stack, 'Template', {
templateFile: path.join(__dirname, 'example-template.json'),
});
const cfnBucket = cfnInclude.getResource('Bucket');
if (cfnBucket.bucketName !== 'my-example-bucket') {
throw new Error(`Expected bucketName to be 'my-example-bucket', got: '${cfnBucket.bucketName}'`);
}

app.synth();
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"Resources": {
"Bucket": {
"Type": "AWS::S3::Bucket",
"Properties": {
"BucketName": "my-example-bucket"
}
}
}
}
15 changes: 15 additions & 0 deletions packages/aws-cdk/test/integ/uberpackage/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module.exports = {
moduleFileExtensions: [
"js",
],
testMatch: [
"**/*.integtest.js",
],
testEnvironment: "node",
bail: 1,
verbose: true,
reporters: [
"default",
[ "jest-junit", { suiteName: "jest tests", outputDirectory: "coverage" } ]
]
};
14 changes: 14 additions & 0 deletions packages/aws-cdk/test/integ/uberpackage/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash

set -euo pipefail

scriptdir=$(cd $(dirname $0) && pwd)

echo '~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~'
echo 'UberCDK Integration Tests'
echo '~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~'

cd $scriptdir

source ../common/jest-test.bash
invokeJest "$@"
12 changes: 12 additions & 0 deletions packages/aws-cdk/test/integ/uberpackage/uberpackage.integtest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { withMonolithicCfnIncludeCdkApp } from '../helpers/cdk';
import { integTest } from '../helpers/test-helpers';

jest.setTimeout(600_000);

describe('uberpackage', () => {
integTest('works with cloudformation-include', withMonolithicCfnIncludeCdkApp(async (fixture) => {
fixture.log('Starting test of cfn-include with monolithic CDK');

await fixture.cdkSynth();
}));
});
4 changes: 2 additions & 2 deletions packages/aws-cdk/test/util/stack-monitor.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { StackActivityMonitor, IActivityPrinter, StackActivity } from '../../lib/api/util/cloudformation/stack-activity-monitor';
import { sleep } from '../integ/cli/aws-helpers';
import { sleep } from '../integ/helpers/aws';
import { MockSdk } from './mock-sdk';

let sdk: MockSdk;
Expand Down Expand Up @@ -167,4 +167,4 @@ async function waitForCondition(cb: () => boolean): Promise<void> {
while (!cb()) {
await sleep(10);
}
}
}
22 changes: 17 additions & 5 deletions tools/ubergen/bin/ubergen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ async function prepareSourceFiles(libraries: readonly LibraryReference[], packag
const indexStatements = new Array<string>();
for (const library of libraries) {
const libDir = path.join(LIB_ROOT, library.shortName);
await transformPackage(library, packageJson.jsii.targets, libDir, libraries);
await transformPackage(library, packageJson, libDir, libraries);

if (library.shortName === 'core') {
indexStatements.push(`export * from './${library.shortName}';`);
Expand All @@ -225,13 +225,13 @@ async function prepareSourceFiles(libraries: readonly LibraryReference[], packag

async function transformPackage(
library: LibraryReference,
config: PackageJson['jsii']['targets'],
uberPackageJson: PackageJson,
destination: string,
allLibraries: readonly LibraryReference[],
) {
await fs.mkdirp(destination);

await copyOrTransformFiles(library.root, destination, allLibraries);
await copyOrTransformFiles(library.root, destination, allLibraries, uberPackageJson);

await fs.writeFile(
path.join(destination, 'index.ts'),
Expand All @@ -240,6 +240,7 @@ async function transformPackage(
);

if (library.shortName !== 'core') {
const config = uberPackageJson.jsii.targets;
await fs.writeJson(
path.join(destination, '.jsiirc.json'),
{
Expand Down Expand Up @@ -291,7 +292,7 @@ function transformTargets(monoConfig: PackageJson['jsii']['targets'], targets: P
return result;
}

async function copyOrTransformFiles(from: string, to: string, libraries: readonly LibraryReference[]) {
async function copyOrTransformFiles(from: string, to: string, libraries: readonly LibraryReference[], uberPackageJson: PackageJson) {
const promises = (await fs.readdir(from)).map(async name => {
if (shouldIgnoreFile(name)) { return; }

Expand All @@ -308,14 +309,25 @@ async function copyOrTransformFiles(from: string, to: string, libraries: readonl
const stat = await fs.stat(source);
if (stat.isDirectory()) {
await fs.mkdirp(destination);
return copyOrTransformFiles(source, destination, libraries);
return copyOrTransformFiles(source, destination, libraries, uberPackageJson);
}
if (name.endsWith('.ts')) {
return fs.writeFile(
destination,
await rewriteImports(source, to, libraries),
{ encoding: 'utf8' },
);
} else if (name === 'cfn-types-2-classes.json') {
// This is a special file used by the cloudformation-include module that contains mappings
// of CFN resource types to the fully-qualified class names of the CDK L1 classes.
// We need to rewrite it to refer to the uberpackage instead of the individual packages
const cfnTypes2Classes: { [key: string]: string } = await fs.readJson(source);
for (const cfnType of Object.keys(cfnTypes2Classes)) {
const fqn = cfnTypes2Classes[cfnType];
// replace @aws-cdk/aws-<service> with <uber-package-name>/aws-<service>
cfnTypes2Classes[cfnType] = fqn.replace('@aws-cdk', uberPackageJson.name);
}
await fs.writeJson(destination, cfnTypes2Classes, { spaces: 2 });
} else {
return fs.copyFile(source, destination);
}
Expand Down

0 comments on commit d40ccda

Please sign in to comment.