Skip to content

Commit

Permalink
fix: remove CloudFormation property renames (aws#973)
Browse files Browse the repository at this point in the history
The construct id parameter used to be a property called `name` in the 
props object, and so properties called `name` were renamed to `xxxName`
(where `xxx` is the name of the resource). Since we now pass names as
a constructor argument, we can get rid of the properly rename mechanism.

BREAKING CHANGE: if you use L1s, you may need to change some
`XxxName` properties back into `Name`. These will match the CloudFormation
property names.

Fixes aws#852.
  • Loading branch information
rix0rrr committed Oct 22, 2018
1 parent 770f9aa commit 3f86603
Show file tree
Hide file tree
Showing 17 changed files with 37 additions and 92 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-apigateway/lib/restapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export class RestApi extends RestApiRef implements cdk.IDependable {
super(parent, id);

const resource = new cloudformation.RestApiResource(this, 'Resource', {
restApiName: props.restApiName || id,
name: props.restApiName || id,
description: props.description,
policy: props.policy,
failOnWarnings: props.failOnWarnings,
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codebuild/lib/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ export class Project extends ProjectRef {
encryptionKey: props.encryptionKey && props.encryptionKey.keyArn,
badgeEnabled: props.badge,
cache,
projectName: props.projectName,
name: props.projectName,
});

this.projectArn = resource.projectArn;
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export class Pipeline extends cdk.Construct implements events.IEventRuleTarget {
stages: new cdk.Token(() => this.renderStages()) as any,
roleArn: this.role.roleArn,
restartExecutionOnUpdate: props && props.restartExecutionOnUpdate,
pipelineName: props && props.pipelineName,
name: props && props.pipelineName,
});

// this will produce a DependsOn for both the role and the policy resources.
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-events/lib/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export class EventRule extends EventRuleRef {
super(parent, name);

const resource = new cloudformation.RuleResource(this, 'Resource', {
ruleName: props.ruleName,
name: props.ruleName,
description: props.description,
state: props.enabled == null ? 'ENABLED' : (props.enabled ? 'ENABLED' : 'DISABLED'),
scheduleExpression: new Token(() => this.scheduleExpression),
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-kinesis/lib/stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ export class Stream extends StreamRef {
const { streamEncryption, encryptionKey } = this.parseEncryption(props);

this.stream = new cloudformation.StreamResource(this, "Resource", {
streamName: props.streamName,
name: props.streamName,
retentionPeriodHours,
shardCount,
streamEncryption
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda/lib/alias.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class Alias extends FunctionRef {
this.underlyingLambda = props.version.lambda;

const alias = new cloudformation.AliasResource(this, 'Resource', {
aliasName: props.aliasName,
name: props.aliasName,
description: props.description,
functionName: this.underlyingLambda.functionName,
functionVersion: props.version.functionVersion,
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-route53/lib/hosted-zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ function toVpcProperty(vpc: ec2.VpcNetworkRef): cloudformation.HostedZoneResourc
}

function determineHostedZoneProps(props: PublicHostedZoneProps) {
const hostedZoneName = props.zoneName + '.';
const name = props.zoneName + '.';
const hostedZoneConfig = props.comment ? { comment: props.comment } : undefined;
const queryLoggingConfig = props.queryLogsLogGroupArn ? { cloudWatchLogsLogGroupArn: props.queryLogsLogGroupArn } : undefined;

return { hostedZoneName, hostedZoneConfig, queryLoggingConfig };
return { name, hostedZoneConfig, queryLoggingConfig };
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-route53/lib/records/txt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class TXTRecord extends Construct {

new cloudformation.RecordSetResource(this, 'Resource', {
hostedZoneId: parent.hostedZoneId,
recordSetName: determineFullyQualifiedDomainName(props.recordName, parent),
name: determineFullyQualifiedDomainName(props.recordName, parent),
type: 'TXT',
resourceRecords: [recordValue],
ttl: ttl.toString()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class ZoneDelegationRecord extends Construct {

new cloudformation.RecordSetResource(this, 'Resource', {
hostedZoneId: parent.hostedZoneId,
recordSetName: determineFullyQualifiedDomainName(props.delegatedZoneName, parent),
name: determineFullyQualifiedDomainName(props.delegatedZoneName, parent),
type: 'NS',
ttl: ttl.toString(),
comment: props.comment,
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { expect, haveResource } from '@aws-cdk/assert';
import cdk = require('@aws-cdk/cdk');
import { Test } from 'nodeunit';
import ssm = require('../lib');

export = {
'association name is rendered properly in L1 construct'(test: Test) {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new ssm.cloudformation.AssociationResource(stack, 'Assoc', {
name: 'document',
});

// THEN
expect(stack).to(haveResource('AWS::SSM::Association', {
Name: 'document',
}));
test.done();
}
};
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-stepfunctions/lib/activity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class Activity extends cdk.Construct implements IStepFunctionsTaskResourc
super(parent, id);

const resource = new cloudformation.ActivityResource(this, 'Resource', {
activityName: props.activityName || this.generateName()
name: props.activityName || this.generateName()
});

this.activityArn = resource.activityArn;
Expand Down
6 changes: 0 additions & 6 deletions packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ export class Resource extends Referenceable {

this.resourceType = props.type;
this.properties = props.properties || { };

// 'name' is a special property included for resource constructs and passed
// as 'name', but we don't want it to be serialized into the template.
if (this.properties.name) {
delete this.properties.name;
}
}

/**
Expand Down
16 changes: 0 additions & 16 deletions packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,22 +240,6 @@ export = {
test.done();
},

'the "name" property is deleted when synthesizing into a CloudFormation resource'(test: Test) {
const stack = new Stack();

new Resource(stack, 'Bla', {
type: 'MyResource',
properties: {
Prop1: 'value1',
name: 'Bla'
}
});

test.deepEqual(stack.toCloudFormation(), { Resources:
{ Bla: { Type: 'MyResource', Properties: { Prop1: 'value1' } } } });
test.done();
},

'removal policy is a high level abstraction of deletion policy used by l2'(test: Test) {
const stack = new Stack();

Expand Down

This file was deleted.

This file was deleted.

2 changes: 1 addition & 1 deletion packages/@aws-cdk/runtime-values/lib/rtv.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class RuntimeValue extends cdk.Construct {
this.parameterName = `/rtv/${new cdk.AwsStackName()}/${props.package}/${name}`;

new ssm.cloudformation.ParameterResource(this, 'Parameter', {
parameterName: this.parameterName,
name: this.parameterName,
type: 'String',
value: props.value,
});
Expand Down
36 changes: 3 additions & 33 deletions tools/cfn2ts/lib/codegen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,28 +144,13 @@ export default class CodeGenerator {
private emitPropsTypeProperties(resourceName: SpecName, propertiesSpec: { [name: string]: schema.Property }): Dictionary<string> {
const propertyMap: Dictionary<string> = {};

// Sanity check that our renamed "Name" is not going to conflict with a real property
const renamedNameProperty = resourceNameProperty(resourceName);
const lowerNames = Object.keys(propertiesSpec).map(s => s.toLowerCase());
if (lowerNames.indexOf('name') !== -1 && lowerNames.indexOf(renamedNameProperty.toLowerCase()) !== -1) {
// tslint:disable-next-line:max-line-length
throw new Error(`Oh gosh, we want to rename ${resourceName.fqn}'s 'Name' property to '${renamedNameProperty}', but that property already exists! We need to find a solution to this problem.`);
}

Object.keys(propertiesSpec).sort(propertyComparator).forEach(propName => {
const originalName = propName;
const propSpec = propertiesSpec[propName];
const additionalDocs = resourceName.relativeName(propName).fqn;

if (propName.toLocaleLowerCase() === 'name') {
propName = renamedNameProperty;
// tslint:disable-next-line:no-console
console.error(`Renamed property 'Name' of ${resourceName.fqn} to '${renamedNameProperty}'`);
}

const resourceCodeName = genspec.CodeName.forResource(resourceName);
const newName = this.emitProperty(resourceCodeName, propName, propSpec, quoteCode(additionalDocs));
propertyMap[originalName] = newName;
propertyMap[propName] = newName;
});
return propertyMap;

Expand Down Expand Up @@ -270,10 +255,9 @@ export default class CodeGenerator {
this.code.line(`super(parent, name, { type: ${resourceName.className}.resourceTypeName${propsType ? ', properties' : ''} });`);
// verify all required properties
if (spec.Properties) {
for (const pname of Object.keys(spec.Properties)) {
const prop = spec.Properties[pname];
for (const propName of Object.keys(spec.Properties)) {
const prop = spec.Properties[propName];
if (prop.Required) {
const propName = pname.toLocaleLowerCase() === 'name' ? resourceNameProperty(resourceName.specName!) : pname;
this.code.line(`${CORE}.requireProperty(properties, '${genspec.cloudFormationToScriptName(propName)}', this);`);
}
}
Expand Down Expand Up @@ -633,20 +617,6 @@ function mapperNames(types: genspec.CodeName[]): string {
return types.map(type => genspec.cfnMapperName(type).fqn).join(', ');
}

/**
* Return the name of the literal "name" property of a resource
*
* A number of resources have a "Name" property. Since Constructs already have a "Name" property
* (which means something different), we must call the original property something else.
*
* We name it after the resource, so for a bucket the "Name" property gets renamed to "BucketName".
*
* (We can leave the name PascalCased, as it's going to be camelCased later).
*/
function resourceNameProperty(resourceName: SpecName) {
return `${resourceName.resourceName}Name`;
}

/**
* Quotes a code name for inclusion in a JSDoc comment, so it will render properly
* in the Sphinx output.
Expand Down

0 comments on commit 3f86603

Please sign in to comment.