From 3f86603d2b64d2605611c84b988c9746a8089e29 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 22 Oct 2018 13:36:01 +0200 Subject: [PATCH] fix: remove CloudFormation property renames (#973) 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 #852. --- .../@aws-cdk/aws-apigateway/lib/restapi.ts | 2 +- .../@aws-cdk/aws-codebuild/lib/project.ts | 2 +- .../@aws-cdk/aws-codepipeline/lib/pipeline.ts | 2 +- packages/@aws-cdk/aws-events/lib/rule.ts | 2 +- packages/@aws-cdk/aws-kinesis/lib/stream.ts | 2 +- packages/@aws-cdk/aws-lambda/lib/alias.ts | 2 +- .../@aws-cdk/aws-route53/lib/hosted-zone.ts | 4 +-- .../@aws-cdk/aws-route53/lib/records/txt.ts | 2 +- .../lib/records/zone-delegation.ts | 2 +- .../aws-ssm/test/test.ssm-document.ts | 22 ++++++++++++ .../aws-stepfunctions/lib/activity.ts | 2 +- .../cdk/lib/cloudformation/resource.ts | 6 ---- .../cdk/test/cloudformation/test.resource.ts | 16 --------- .../500_SSM_AssociationName_patch.json | 16 --------- .../500_SSM_AssociationName_patch.md | 9 ----- packages/@aws-cdk/runtime-values/lib/rtv.ts | 2 +- tools/cfn2ts/lib/codegen.ts | 36 ++----------------- 17 files changed, 37 insertions(+), 92 deletions(-) create mode 100644 packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts delete mode 100644 packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json delete mode 100644 packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md diff --git a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts index e9bd84f2422c9..a0db6063b556e 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts @@ -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, diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index fdb2573271636..5a0f73a7027b8 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -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; diff --git a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts index 3672521b19b56..04d653a000064 100644 --- a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts +++ b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts @@ -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. diff --git a/packages/@aws-cdk/aws-events/lib/rule.ts b/packages/@aws-cdk/aws-events/lib/rule.ts index c2214a0257aa2..3d9ff6716b18f 100644 --- a/packages/@aws-cdk/aws-events/lib/rule.ts +++ b/packages/@aws-cdk/aws-events/lib/rule.ts @@ -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), diff --git a/packages/@aws-cdk/aws-kinesis/lib/stream.ts b/packages/@aws-cdk/aws-kinesis/lib/stream.ts index 1ba79398612b5..648b2ee498779 100644 --- a/packages/@aws-cdk/aws-kinesis/lib/stream.ts +++ b/packages/@aws-cdk/aws-kinesis/lib/stream.ts @@ -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 diff --git a/packages/@aws-cdk/aws-lambda/lib/alias.ts b/packages/@aws-cdk/aws-lambda/lib/alias.ts index 6665ae4681fcb..297cfad57b825 100644 --- a/packages/@aws-cdk/aws-lambda/lib/alias.ts +++ b/packages/@aws-cdk/aws-lambda/lib/alias.ts @@ -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, diff --git a/packages/@aws-cdk/aws-route53/lib/hosted-zone.ts b/packages/@aws-cdk/aws-route53/lib/hosted-zone.ts index 6ed342f66beaf..c80062265f422 100644 --- a/packages/@aws-cdk/aws-route53/lib/hosted-zone.ts +++ b/packages/@aws-cdk/aws-route53/lib/hosted-zone.ts @@ -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 }; } diff --git a/packages/@aws-cdk/aws-route53/lib/records/txt.ts b/packages/@aws-cdk/aws-route53/lib/records/txt.ts index 55171ed308495..9ab789c7ec75a 100644 --- a/packages/@aws-cdk/aws-route53/lib/records/txt.ts +++ b/packages/@aws-cdk/aws-route53/lib/records/txt.ts @@ -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() diff --git a/packages/@aws-cdk/aws-route53/lib/records/zone-delegation.ts b/packages/@aws-cdk/aws-route53/lib/records/zone-delegation.ts index c3e093afc8494..3bf7c255494f6 100644 --- a/packages/@aws-cdk/aws-route53/lib/records/zone-delegation.ts +++ b/packages/@aws-cdk/aws-route53/lib/records/zone-delegation.ts @@ -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, diff --git a/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts b/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts new file mode 100644 index 0000000000000..b6da8688e5976 --- /dev/null +++ b/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts @@ -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(); + } +}; \ No newline at end of file diff --git a/packages/@aws-cdk/aws-stepfunctions/lib/activity.ts b/packages/@aws-cdk/aws-stepfunctions/lib/activity.ts index a35422ac352b3..505e4b14405ef 100644 --- a/packages/@aws-cdk/aws-stepfunctions/lib/activity.ts +++ b/packages/@aws-cdk/aws-stepfunctions/lib/activity.ts @@ -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; diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts b/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts index a8c90de6e6348..e53d11112fbd8 100644 --- a/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts +++ b/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts @@ -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; - } } /** diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts index 4c5d303c97848..7c9fa4a60c722 100644 --- a/packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts +++ b/packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts @@ -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(); diff --git a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json b/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json deleted file mode 100644 index 8ce9d9a3deb39..0000000000000 --- a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "ResourceTypes": { - "AWS::SSM::Association": { - "patch": { - "description": "rename +Name+ to +DocumentName+ to clearly indicate what the attribute is for", - "operations": [ - { - "from": "/Properties/Name", - "op": "move", - "path": "/Properties/DocumentName" - } - ] - } - } - } -} diff --git a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md b/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md deleted file mode 100644 index 9c311b0dbe502..0000000000000 --- a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md +++ /dev/null @@ -1,9 +0,0 @@ -We discovered that SSM::Assocations have an AssociationName and a Name parameter, which introduced a conflict. - -Apart from that we have to resolve the conflict, the original heuristics were even wrong: - -- Name is NOT the name of the resource to be created, it's the name of the SSM document to associate with. -- AssociationName IS the name of the resource to be created, that's fine, it's the name that we would have -picked for that attribute anyway. - -We rename "Name" to "DocumentName" to clearly indicate what the Name attribute is for. diff --git a/packages/@aws-cdk/runtime-values/lib/rtv.ts b/packages/@aws-cdk/runtime-values/lib/rtv.ts index 778da7666fe92..bf2fbc7bd6fc6 100644 --- a/packages/@aws-cdk/runtime-values/lib/rtv.ts +++ b/packages/@aws-cdk/runtime-values/lib/rtv.ts @@ -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, }); diff --git a/tools/cfn2ts/lib/codegen.ts b/tools/cfn2ts/lib/codegen.ts index ffd18a56ef6ec..e47762e7a4e90 100644 --- a/tools/cfn2ts/lib/codegen.ts +++ b/tools/cfn2ts/lib/codegen.ts @@ -144,28 +144,13 @@ export default class CodeGenerator { private emitPropsTypeProperties(resourceName: SpecName, propertiesSpec: { [name: string]: schema.Property }): Dictionary { const propertyMap: Dictionary = {}; - // 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; @@ -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);`); } } @@ -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.