From e537e4c6daa013ca506a1a8c7f3d7637f179d131 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Mon, 24 Jun 2019 17:14:27 +0300 Subject: [PATCH] refactor(core): CfnResource.options => cfnOptions (#3030) In order to avoid future conflicts with L1 properties, we prefix all properties of CfnResource with `cfn`. BREAKING CHANGE: The `CfnResource.options` property was renamed to `CfnResource.cfnOptions` to avoid conflicts with properties introduced by derived classes. --- .../@aws-cdk/aws-apigateway/lib/deployment.ts | 2 +- .../aws-autoscaling/lib/auto-scaling-group.ts | 20 ++++++------- .../lib/lambda/deployment-group.ts | 2 +- .../aws-iam/test/test.escape-hatch.ts | 2 +- packages/@aws-cdk/aws-s3-assets/lib/asset.ts | 6 ++-- packages/@aws-cdk/core/lib/cfn-resource.ts | 30 ++++++++++++------- packages/@aws-cdk/core/test/test.resource.ts | 14 ++++----- 7 files changed, 42 insertions(+), 34 deletions(-) diff --git a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts index 32300635a58b1..3aecccee8c090 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts @@ -72,7 +72,7 @@ export class Deployment extends Resource { }); if (props.retainDeployments) { - this.resource.options.deletionPolicy = CfnDeletionPolicy.RETAIN; + this.resource.cfnOptions.deletionPolicy = CfnDeletionPolicy.RETAIN; } this.api = props.api; diff --git a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts index 1891673d5d63e..6594dc67249e5 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts @@ -518,8 +518,8 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements */ private applyUpdatePolicies(props: AutoScalingGroupProps) { if (props.updateType === UpdateType.REPLACING_UPDATE) { - this.autoScalingGroup.options.updatePolicy = { - ...this.autoScalingGroup.options.updatePolicy, + this.autoScalingGroup.cfnOptions.updatePolicy = { + ...this.autoScalingGroup.cfnOptions.updatePolicy, autoScalingReplacingUpdate: { willReplace: true } @@ -531,31 +531,31 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements // during the update? // // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-attribute-creationpolicy.html - this.autoScalingGroup.options.creationPolicy = { - ...this.autoScalingGroup.options.creationPolicy, + this.autoScalingGroup.cfnOptions.creationPolicy = { + ...this.autoScalingGroup.cfnOptions.creationPolicy, autoScalingCreationPolicy: { minSuccessfulInstancesPercent: validatePercentage(props.replacingUpdateMinSuccessfulInstancesPercent) } }; } } else if (props.updateType === UpdateType.ROLLING_UPDATE) { - this.autoScalingGroup.options.updatePolicy = { - ...this.autoScalingGroup.options.updatePolicy, + this.autoScalingGroup.cfnOptions.updatePolicy = { + ...this.autoScalingGroup.cfnOptions.updatePolicy, autoScalingRollingUpdate: renderRollingUpdateConfig(props.rollingUpdateConfiguration) }; } // undefined is treated as 'true' if (props.ignoreUnmodifiedSizeProperties !== false) { - this.autoScalingGroup.options.updatePolicy = { - ...this.autoScalingGroup.options.updatePolicy, + this.autoScalingGroup.cfnOptions.updatePolicy = { + ...this.autoScalingGroup.cfnOptions.updatePolicy, autoScalingScheduledAction: { ignoreUnmodifiedGroupSizeProperties: true } }; } if (props.resourceSignalCount !== undefined || props.resourceSignalTimeout !== undefined) { - this.autoScalingGroup.options.creationPolicy = { - ...this.autoScalingGroup.options.creationPolicy, + this.autoScalingGroup.cfnOptions.creationPolicy = { + ...this.autoScalingGroup.cfnOptions.creationPolicy, resourceSignal: { count: props.resourceSignalCount, timeout: props.resourceSignalTimeout && props.resourceSignalTimeout.toISOString(), diff --git a/packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts b/packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts index 876fe2c3645f8..0fcddbb7526ee 100644 --- a/packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts +++ b/packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts @@ -183,7 +183,7 @@ export class LambdaDeploymentGroup extends cdk.Resource implements ILambdaDeploy this.addPostHook(props.postHook); } - (props.alias.node.defaultChild as lambda.CfnAlias).options.updatePolicy = { + (props.alias.node.defaultChild as lambda.CfnAlias).cfnOptions.updatePolicy = { codeDeployLambdaAliasUpdate: { applicationName: this.application.applicationName, deploymentGroupName: resource.ref, diff --git a/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts b/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts index b12bcaee0a4ec..cb7d36c6503cb 100644 --- a/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts +++ b/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts @@ -61,7 +61,7 @@ export = { const stack = new Stack(); const user = new iam.User(stack, 'user', { userName: 'MyUserName' }); const cfn = user.node.findChild('Resource') as iam.CfnUser; - cfn.options.updatePolicy = { useOnlineResharding: true }; + cfn.cfnOptions.updatePolicy = { useOnlineResharding: true }; // WHEN cfn.addOverride('Properties.Hello.World', 'Bam'); diff --git a/packages/@aws-cdk/aws-s3-assets/lib/asset.ts b/packages/@aws-cdk/aws-s3-assets/lib/asset.ts index 5512fe6a122dd..1b3a8df9599db 100644 --- a/packages/@aws-cdk/aws-s3-assets/lib/asset.ts +++ b/packages/@aws-cdk/aws-s3-assets/lib/asset.ts @@ -169,9 +169,9 @@ export class Asset extends cdk.Construct implements assets.IAsset { // tell tools such as SAM CLI that the "Code" property of this resource // points to a local path in order to enable local invocation of this function. - resource.options.metadata = resource.options.metadata || { }; - resource.options.metadata[cxapi.ASSET_RESOURCE_METADATA_PATH_KEY] = this.assetPath; - resource.options.metadata[cxapi.ASSET_RESOURCE_METADATA_PROPERTY_KEY] = resourceProperty; + resource.cfnOptions.metadata = resource.cfnOptions.metadata || { }; + resource.cfnOptions.metadata[cxapi.ASSET_RESOURCE_METADATA_PATH_KEY] = this.assetPath; + resource.cfnOptions.metadata[cxapi.ASSET_RESOURCE_METADATA_PROPERTY_KEY] = resourceProperty; } /** diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index 206a015c352a7..edb7d3f87a8ea 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -36,10 +36,18 @@ export class CfnResource extends CfnRefElement { return (construct as any).cfnResourceType !== undefined; } + // MAINTAINERS NOTE: this class serves as the base class for the generated L1 + // ("CFN") resources (such as `s3.CfnBucket`). These resources will have a + // property for each CloudFormation property of the resource. This means that + // if at some point in the future a property is introduced with a name similar + // to one of the properties here, it will be "masked" by the derived class. To + // that end, we prefix all properties in this class with `cfnXxx` with the + // hope to avoid those conflicts in the future. + /** * Options for this resource, such as condition, update policy etc. */ - public readonly options: IResourceOptions = {}; + public readonly cfnOptions: ICfnResourceOptions = {}; /** * AWS resource type. @@ -84,7 +92,7 @@ export class CfnResource extends CfnRefElement { // path in the CloudFormation template, so it will be possible to trace // back to the actual construct path. if (this.node.tryGetContext(cxapi.PATH_METADATA_ENABLE_CONTEXT)) { - this.options.metadata = { + this.cfnOptions.metadata = { [cxapi.PATH_METADATA_KEY]: this.node.path }; } @@ -111,9 +119,9 @@ export class CfnResource extends CfnRefElement { throw new Error(`Invalid removal policy: ${policy}`); } - this.options.deletionPolicy = deletionPolicy; + this.cfnOptions.deletionPolicy = deletionPolicy; if (options.applyToUpdateReplacePolicy) { - this.options.updateReplacePolicy = deletionPolicy; + this.cfnOptions.updateReplacePolicy = deletionPolicy; } } @@ -215,12 +223,12 @@ export class CfnResource extends CfnRefElement { Type: this.cfnResourceType, Properties: ignoreEmpty(this.cfnProperties), DependsOn: ignoreEmpty(renderDependsOn(this.dependsOn)), - CreationPolicy: capitalizePropertyNames(this, renderCreationPolicy(this.options.creationPolicy)), - UpdatePolicy: capitalizePropertyNames(this, this.options.updatePolicy), - UpdateReplacePolicy: capitalizePropertyNames(this, this.options.updateReplacePolicy), - DeletionPolicy: capitalizePropertyNames(this, this.options.deletionPolicy), - Metadata: ignoreEmpty(this.options.metadata), - Condition: this.options.condition && this.options.condition.logicalId + CreationPolicy: capitalizePropertyNames(this, renderCreationPolicy(this.cfnOptions.creationPolicy)), + UpdatePolicy: capitalizePropertyNames(this, this.cfnOptions.updatePolicy), + UpdateReplacePolicy: capitalizePropertyNames(this, this.cfnOptions.updateReplacePolicy), + DeletionPolicy: capitalizePropertyNames(this, this.cfnOptions.deletionPolicy), + Metadata: ignoreEmpty(this.cfnOptions.metadata), + Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { props.Properties = this.renderProperties(props.Properties); return deepMerge(props, this.rawOverrides); @@ -294,7 +302,7 @@ export enum TagType { NOT_TAGGABLE = 'NotTaggable', } -export interface IResourceOptions { +export interface ICfnResourceOptions { /** * A condition to associate with this resource. This means that only if the condition evaluates to 'true' when the stack * is deployed, the resource will be included. This is provided to allow CDK projects to produce legacy templates, but noramlly diff --git a/packages/@aws-cdk/core/test/test.resource.ts b/packages/@aws-cdk/core/test/test.resource.ts index 9a9fab6daacee..92cab3f7df156 100644 --- a/packages/@aws-cdk/core/test/test.resource.ts +++ b/packages/@aws-cdk/core/test/test.resource.ts @@ -190,7 +190,7 @@ export = { const stack = new Stack(); const r1 = new CfnResource(stack, 'Resource', { type: 'Type' }); const cond = new CfnCondition(stack, 'MyCondition', { expression: Fn.conditionNot(Fn.conditionEquals('a', 'b')) }); - r1.options.condition = cond; + r1.cfnOptions.condition = cond; test.deepEqual(toCloudFormation(stack), { Resources: { Resource: { Type: 'Type', Condition: 'MyCondition' } }, @@ -204,9 +204,9 @@ export = { const stack = new Stack(); const r1 = new CfnResource(stack, 'Resource', { type: 'Type' }); - r1.options.creationPolicy = { autoScalingCreationPolicy: { minSuccessfulInstancesPercent: 10 } }; + r1.cfnOptions.creationPolicy = { autoScalingCreationPolicy: { minSuccessfulInstancesPercent: 10 } }; // tslint:disable-next-line:max-line-length - r1.options.updatePolicy = { + r1.cfnOptions.updatePolicy = { autoScalingScheduledAction: { ignoreUnmodifiedGroupSizeProperties: false }, autoScalingReplacingUpdate: { willReplace: true }, codeDeployLambdaAliasUpdate: { @@ -215,8 +215,8 @@ export = { beforeAllowTrafficHook: 'lambda1', }, }; - r1.options.deletionPolicy = CfnDeletionPolicy.RETAIN; - r1.options.updateReplacePolicy = CfnDeletionPolicy.SNAPSHOT; + r1.cfnOptions.deletionPolicy = CfnDeletionPolicy.RETAIN; + r1.cfnOptions.updateReplacePolicy = CfnDeletionPolicy.SNAPSHOT; test.deepEqual(toCloudFormation(stack), { Resources: { @@ -245,7 +245,7 @@ export = { const stack = new Stack(); const r1 = new CfnResource(stack, 'Resource', { type: 'Type' }); - r1.options.updatePolicy = { useOnlineResharding: true }; + r1.cfnOptions.updatePolicy = { useOnlineResharding: true }; test.deepEqual(toCloudFormation(stack), { Resources: { @@ -265,7 +265,7 @@ export = { const stack = new Stack(); const r1 = new CfnResource(stack, 'Resource', { type: 'Type' }); - r1.options.metadata = { + r1.cfnOptions.metadata = { MyKey: 10, MyValue: 99 };