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

chore: do not use "synthesize" and "prepare" in the cdk #9410

Merged
merged 10 commits into from
Aug 4, 2020
52 changes: 26 additions & 26 deletions packages/@aws-cdk/aws-apigateway/lib/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ interface LatestDeploymentResourceProps {

class LatestDeploymentResource extends CfnDeployment {
private hashComponents = new Array<any>();
private originalLogicalId: string;

private api: IRestApi;

constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) {
Expand All @@ -133,7 +133,31 @@ class LatestDeploymentResource extends CfnDeployment {
});

this.api = props.restApi;
this.originalLogicalId = Stack.of(this).getLogicalId(this);

const originalLogicalId = Stack.of(this).getLogicalId(this);

this.overrideLogicalId(Lazy.stringValue({ produce: ctx => {
const hash = [ ...this.hashComponents ];

if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported

// Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
hash.push(ctx.resolve(cfnRestApiCF));
iliapolo marked this conversation as resolved.
Show resolved Hide resolved
}

let lid = originalLogicalId;

// if hash components were added to the deployment, we use them to calculate
// a logical ID for the deployment resource.
if (hash.length > 0) {
const md5 = crypto.createHash('md5');
hash.map(x => ctx.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
iliapolo marked this conversation as resolved.
Show resolved Hide resolved
lid += md5.digest('hex');
}

return lid;
}}));
}

/**
Expand All @@ -149,28 +173,4 @@ class LatestDeploymentResource extends CfnDeployment {

this.hashComponents.push(data);
}

/**
* Hooks into synthesis to calculate a logical ID that hashes all the components
* add via `addToLogicalId`.
*/
protected prepare() {
if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported

// Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
this.addToLogicalId(Stack.of(this).resolve(cfnRestApiCF));
}

const stack = Stack.of(this);

// if hash components were added to the deployment, we use them to calculate
// a logical ID for the deployment resource.
if (this.hashComponents.length > 0) {
const md5 = crypto.createHash('md5');
this.hashComponents.map(x => stack.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
this.overrideLogicalId(this.originalLogicalId + md5.digest('hex'));
}
super.prepare();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
"BooksApi60AC975F"
]
},
"BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0": {
"BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be": {
"Type": "AWS::ApiGateway::Deployment",
"Properties": {
"RestApiId": {
Expand All @@ -141,7 +141,7 @@
"Ref": "BooksApi60AC975F"
},
"DeploymentId": {
"Ref": "BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0"
"Ref": "BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be"
},
"StageName": "prod"
}
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-apigateway/test/test.deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,16 @@ export = {
// the logical ID changed
const template = synthesize();
test.ok(!template.Resources.deployment33381975bba46c5132329b81e7befcbbba5a0e75, 'old resource id is not deleted');
test.ok(template.Resources.deployment33381975075f46a4503208d69fcffed2f263c48c,
`new resource deployment33381975075f46a4503208d69fcffed2f263c48c is not created, instead found ${Object.keys(template.Resources)}`);
test.ok(template.Resources.deployment333819758aa4cdb9d204502b959c4903f4d5d29f,
`new resource deployment333819758aa4cdb9d204502b959c4903f4d5d29f is not created, instead found ${Object.keys(template.Resources)}`);

// tokens supported, and are resolved upon synthesis
const value = 'hello hello';
deployment.addToLogicalId({ foo: Lazy.stringValue({ produce: () => value }) });

const template2 = synthesize();
test.ok(template2.Resources.deployment33381975b6d7672e4c9afd0b741e41d07739786b,
`resource deployment33381975b6d7672e4c9afd0b741e41d07739786b not found, instead found ${Object.keys(template2.Resources)}`);
test.ok(template2.Resources.deployment333819758d91bed959c6bd6268ba84f6d33e888e,
`resource deployment333819758d91bed959c6bd6268ba84f6d33e888e not found, instead found ${Object.keys(template2.Resources)}`);

test.done();

Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ describe('with Lambda@Edge functions', () => {
{
EventType: 'origin-request',
LambdaFunctionARN: {
Ref: 'FunctionCurrentVersion4E2B22619c0305f954e58f25575548280c0a3629',
Ref: 'FunctionCurrentVersion4E2B2261477a5ae8059bbaa7813f752292c0f65e',
},
},
],
Expand Down Expand Up @@ -341,7 +341,7 @@ describe('with Lambda@Edge functions', () => {
{
EventType: 'viewer-request',
LambdaFunctionARN: {
Ref: 'FunctionCurrentVersion4E2B22619c0305f954e58f25575548280c0a3629',
Ref: 'FunctionCurrentVersion4E2B2261477a5ae8059bbaa7813f752292c0f65e',
},
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export = nodeunit.testCase({
actions: [action],
});

cdk.ConstructNode.prepare(stack.node);
app.synth();

_assertPermissionGranted(test, stack, pipelineRole.statements, 'iam:PassRole', action.deploymentRole.roleArn);

Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-ec2/test/connections.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, haveResource } from '@aws-cdk/assert';
import { App, ConstructNode, Stack } from '@aws-cdk/core';
import { App, Stack } from '@aws-cdk/core';
import { nodeunitShim, Test } from 'nodeunit-shim';

import {
Expand Down Expand Up @@ -185,7 +185,7 @@ nodeunitShim({
sg2.connections.allowFrom(sg1, Port.tcp(100));

// THEN -- both rules are in Stack2
ConstructNode.prepare(app.node);
app.synth();

expect(stack2).to(haveResource('AWS::EC2::SecurityGroupIngress', {
GroupId: { 'Fn::GetAtt': [ 'SecurityGroupDD263621', 'GroupId' ] },
Expand Down Expand Up @@ -216,7 +216,7 @@ nodeunitShim({
sg2.connections.allowTo(sg1, Port.tcp(100));

// THEN -- both rules are in Stack2
ConstructNode.prepare(app.node);
app.synth();

expect(stack2).to(haveResource('AWS::EC2::SecurityGroupIngress', {
GroupId: { 'Fn::ImportValue': 'Stack1:ExportsOutputFnGetAttSecurityGroupDD263621GroupIdDF6F8B09' },
Expand Down Expand Up @@ -249,7 +249,7 @@ nodeunitShim({
sg2.connections.allowFrom(sg1b, Port.tcp(100));

// THEN -- both egress rules are in Stack2
ConstructNode.prepare(app.node);
app.synth();

expect(stack2).to(haveResource('AWS::EC2::SecurityGroupEgress', {
GroupId: { 'Fn::ImportValue': 'Stack1:ExportsOutputFnGetAttSecurityGroupAED40ADC5GroupId1D10C76A' },
Expand Down
29 changes: 12 additions & 17 deletions packages/@aws-cdk/aws-lambda/lib/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,18 @@ export class Function extends FunctionBase {
...this.currentVersionOptions,
});

// override the version's logical ID with a lazy string which includes the
// hash of the function itself, so a new version resource is created when
// the function configuration changes.
const cfn = this._currentVersion.node.defaultChild as CfnResource;
const originalLogicalId = this.stack.resolve(cfn.logicalId) as string;

cfn.overrideLogicalId(Lazy.stringValue({ produce: _ => {
const hash = calculateFunctionHash(this);
const logicalId = trimFromStart(originalLogicalId, 255 - 32);
return `${logicalId}${hash}`;
}}));

return this._currentVersion;
}

Expand Down Expand Up @@ -739,23 +751,6 @@ export class Function extends FunctionBase {
return this._logGroup;
}

protected prepare() {
super.prepare();

// if we have a current version resource, override it's logical id
// so that it includes the hash of the function code and it's configuration.
if (this._currentVersion) {
const stack = Stack.of(this);
const cfn = this._currentVersion.node.defaultChild as CfnResource;
const originalLogicalId: string = stack.resolve(cfn.logicalId);

const hash = calculateFunctionHash(this);

const logicalId = trimFromStart(originalLogicalId, 255 - 32);
cfn.overrideLogicalId(`${logicalId}${hash}`);
}
}

private renderEnvironment() {
if (!this.environment || Object.keys(this.environment).length === 0) {
return undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
"MyLambdaServiceRole4539ECB6"
]
},
"MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df": {
"MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e": {
"Type": "AWS::Lambda::Version",
"Properties": {
"FunctionName": {
Expand All @@ -103,7 +103,7 @@
},
"Qualifier": {
"Fn::GetAtt": [
"MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df",
"MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e",
"Version"
]
},
Expand All @@ -118,7 +118,7 @@
},
"FunctionVersion": {
"Fn::GetAtt": [
"MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df",
"MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e",
"Version"
]
},
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda/test/test.lambda-version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export = {
},
'FunctionVersion': {
'Fn::GetAtt': [
'FnCurrentVersion17A89ABB19ed45993ff69fd011ae9fd4ab6e2005',
'FnCurrentVersion17A89ABBab5c765f3c55e4e61583b51b00a95742',
'Version',
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,6 @@ test('a notification destination can specify a set of dependencies that must be

bucket.addObjectCreatedNotification(dest);

cdk.ConstructNode.prepare(stack.node);

expect(SynthUtils.synthesize(stack).template.Resources.BucketNotifications8F2E257D).toEqual({
Type: 'Custom::S3BucketNotifications',
Properties: {
Expand Down
22 changes: 18 additions & 4 deletions packages/@aws-cdk/core/lib/private/synthesis.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import * as cxapi from '@aws-cdk/cx-api';
import * as constructs from 'constructs';
import { Construct, IConstruct, SynthesisOptions, ValidationError } from '../construct-compat';
import { Stack } from '../stack';
import { Stage, StageSynthesisOptions } from '../stage';
import { prepareApp } from './prepare-app';
import { TreeMetadata } from './tree-metadata';

export function synthesize(root: IConstruct, options: SynthesisOptions = { }): cxapi.CloudAssembly {
// we start by calling "synth" on all nested assemblies (which will take care of all their children)
Expand Down Expand Up @@ -103,10 +105,22 @@ function prepareTree(root: IConstruct) {
* Stop at Assembly boundaries.
*/
function synthesizeTree(root: IConstruct, builder: cxapi.CloudAssemblyBuilder) {
visit(root, 'post', construct => construct.onSynthesize({
outdir: builder.outdir,
assembly: builder,
}));
visit(root, 'post', construct => {
const session = {
outdir: builder.outdir,
assembly: builder,
};

if (construct instanceof Stack) {
construct._synthesizeTemplate(session);
Copy link
Contributor

Choose a reason for hiding this comment

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

So, theoretically, if a user implemented synthesize on the stack level, we won't be calling it anymore - right? That sounds like a breaking change, am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still calling synthesize through the onSynthesize() shim.

Copy link
Contributor

@iliapolo iliapolo Aug 4, 2020

Choose a reason for hiding this comment

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

Not sure I follow - where? I see we are calling onSynthesize on constructs that aren't a stack, but what about those who are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm. okay let me double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, nice catch!

} else if (construct instanceof TreeMetadata) {
construct._synthesizeTree(session);
}

// this will soon be deprecated and removed in 2.x
// see https://github.com/aws/aws-cdk-rfcs/issues/192
construct.onSynthesize(session);
});
}

/**
Expand Down
6 changes: 5 additions & 1 deletion packages/@aws-cdk/core/lib/private/tree-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ export class TreeMetadata extends Construct {
super(scope, 'Tree');
}

protected synthesize(session: ISynthesisSession) {
/**
* Create tree.json
* @internal
*/
public _synthesizeTree(session: ISynthesisSession) {
const lookup: { [path: string]: Node } = { };

const visit = (construct: IConstruct): Node => {
Expand Down
48 changes: 26 additions & 22 deletions packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,32 @@ export class Stack extends Construct implements ITaggable {
}
}

/**
* Synthesizes the cloudformation template into a cloud assembly.
* @internal
*/
public _synthesizeTemplate(session: ISynthesisSession): void {
// In principle, stack synthesis is delegated to the
// StackSynthesis object.
//
// However, some parts of synthesis currently use some private
// methods on Stack, and I don't really see the value in refactoring
// this right now, so some parts still happen here.
const builder = session.assembly;

// write the CloudFormation template as a JSON file
const outPath = path.join(builder.outdir, this.templateFile);
const text = JSON.stringify(this._toCloudFormation(), undefined, 2);
fs.writeFileSync(outPath, text);

for (const ctx of this._missingContext) {
builder.addMissing(ctx);
}

// Delegate adding artifacts to the Synthesizer
this.synthesizer.synthesizeStackArtifacts(session);
}

/**
* Returns the naming scheme used to allocate logical IDs. By default, uses
* the `HashedAddressingScheme` but this method can be overridden to customize
Expand Down Expand Up @@ -761,28 +787,6 @@ export class Stack extends Construct implements ITaggable {
}
}

protected synthesize(session: ISynthesisSession): void {
// In principle, stack synthesis is delegated to the
// StackSynthesis object.
//
// However, some parts of synthesis currently use some private
// methods on Stack, and I don't really see the value in refactoring
// this right now, so some parts still happen here.
const builder = session.assembly;

// write the CloudFormation template as a JSON file
const outPath = path.join(builder.outdir, this.templateFile);
const text = JSON.stringify(this._toCloudFormation(), undefined, 2);
fs.writeFileSync(outPath, text);

for (const ctx of this._missingContext) {
builder.addMissing(ctx);
}

// Delegate adding artifacts to the Synthesizer
this.synthesizer.synthesizeStackArtifacts(session);
}

/**
* Returns the CloudFormation template for this stack by traversing
* the tree and invoking _toCloudFormation() on all Entity objects.
Expand Down
Loading