From 3605f9cfb0af314c6416e4bcc17895e781977530 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 25 Oct 2018 12:42:16 +0200 Subject: [PATCH] fix(cloudformation-diff): ignore changes to DependsOn (#1005) Don't consider absence of changes to Properties to be certain changes to Type (other things that could have changed are for example DependsOn and UpdatePolicy). Check for that explicitly. Fixes #274. --- .../cloudformation-diff/lib/diff/index.ts | 29 ++++++------ .../cloudformation-diff/lib/diff/types.ts | 31 +++++++++---- .../cloudformation-diff/lib/format.ts | 6 +-- .../test/test.diff-template.ts | 46 +++++++++++++++---- 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts index 9841e5d177bf9..b8c744120fa27 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts @@ -27,25 +27,22 @@ export function diffParameter(oldValue: types.Parameter, newValue: types.Paramet } export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference { - let resourceType: string | { oldType: string, newType: string } = - ((oldValue && oldValue.Type) || (newValue && newValue.Type)) as string; + const resourceType = { + oldType: oldValue && oldValue.Type, + newType: newValue && newValue.Type + }; let propertyChanges: { [key: string]: types.PropertyDifference } = {}; let otherChanges: { [key: string]: types.Difference } = {}; - if (oldValue && newValue) { - const oldType = oldValue.Type as string; - const newType = newValue.Type as string; - if (oldType !== newType) { - resourceType = { oldType, newType }; - } else { - const typeSpec = cfnspec.filteredSpecification(oldType); - const impl = typeSpec.ResourceTypes[oldType]; - propertyChanges = diffKeyedEntities(oldValue.Properties, - newValue.Properties, - (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl)); - otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther); - delete otherChanges.Properties; - } + if (resourceType.oldType === resourceType.newType) { + // Only makes sense to inspect deeper if the types stayed the same + const typeSpec = cfnspec.filteredSpecification(resourceType.oldType); + const impl = typeSpec.ResourceTypes[resourceType.oldType]; + propertyChanges = diffKeyedEntities(oldValue.Properties, + newValue.Properties, + (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl)); + otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther); + delete otherChanges.Properties; } return new types.ResourceDifference(oldValue, newValue, { resourceType, propertyChanges, otherChanges }); diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 5f53523c0aed0..f921531e39919 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -210,17 +210,18 @@ export interface Resource { [key: string]: any; } export class ResourceDifference extends Difference { - /** The resource type (or old and new type if it has changed) */ - public readonly resourceType: string | { readonly oldType: string, readonly newType: string }; /** Property-level changes on the resource */ public readonly propertyChanges: { [key: string]: PropertyDifference }; /** Changes to non-property level attributes of the resource */ public readonly otherChanges: { [key: string]: Difference }; + /** The resource type (or old and new type if it has changed) */ + private readonly resourceType: { readonly oldType: string, readonly newType: string }; + constructor(oldValue: Resource | undefined, newValue: Resource | undefined, args: { - resourceType: string | { oldType: string, newType: string }, + resourceType: { oldType: string, newType: string }, propertyChanges: { [key: string]: Difference }, otherChanges: { [key: string]: Difference } } @@ -231,14 +232,26 @@ export class ResourceDifference extends Difference { this.otherChanges = args.otherChanges; } + public get oldResourceType(): string | undefined { + return this.resourceType.oldType; + } + + public get newResourceType(): string | undefined { + return this.resourceType.newType; + } + public get changeImpact(): ResourceImpact { - if (Object.keys(this.propertyChanges).length === 0) { - if (typeof this.resourceType !== 'string') { return ResourceImpact.WILL_REPLACE; } - if (!this.oldValue) { return ResourceImpact.WILL_CREATE; } - return this.oldValue.DeletionPolicy === 'Retain' - ? ResourceImpact.WILL_ORPHAN - : ResourceImpact.WILL_DESTROY; + // Check the Type first + if (this.resourceType.oldType !== this.resourceType.newType) { + if (this.resourceType.oldType === undefined) { return ResourceImpact.WILL_CREATE; } + if (this.resourceType.newType === undefined) { + return this.oldValue!.DeletionPolicy === 'Retain' + ? ResourceImpact.WILL_ORPHAN + : ResourceImpact.WILL_DESTROY; + } + return ResourceImpact.WILL_REPLACE; } + return Object.values(this.propertyChanges) .map(elt => elt.changeImpact) .reduce(worstImpact, ResourceImpact.WILL_UPDATE); diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 8dffa4361462f..3415a9b75237e 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -60,13 +60,13 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp ADDITION, formatImpact(diff.changeImpact), colors.blue(logicalId), - formatValue(diff.resourceType, colors.green)); + formatValue(diff.newResourceType, colors.green)); } else if (diff.isUpdate) { print('%s %s %s (type: %s)', UPDATE, formatImpact(diff.changeImpact), colors.blue(logicalId), - formatValue(diff.resourceType, colors.green)); + formatValue(diff.newResourceType, colors.green)); let processedCount = 0; diff.forEach((type, name, values) => { processedCount += 1; @@ -78,7 +78,7 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp REMOVAL, formatImpact(diff.changeImpact), colors.blue(logicalId), - formatValue(diff.resourceType, colors.green)); + formatValue(diff.oldResourceType, colors.green)); } } diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts index 32c264bead1b1..c4e4d2e1f8a42 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts @@ -45,7 +45,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); test.ok(difference && difference.isAddition, 'the difference reflects there was no such resource before'); - test.deepEqual(difference && difference.resourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); + test.deepEqual(difference && difference.newResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.equal(difference && difference.changeImpact, ResourceImpact.WILL_CREATE, 'the difference reflects that a new resource will be created'); test.done(); }, @@ -70,7 +70,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketPolicyResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketPolicyResource logical ID'); test.ok(difference && difference.isRemoval, 'the difference reflects there is no such resource after'); - test.deepEqual(difference && difference.resourceType, 'AWS::S3::BucketPolicy', 'the difference reports the resource type'); + test.deepEqual(difference && difference.oldResourceType, 'AWS::S3::BucketPolicy', 'the difference reports the resource type'); test.equal(difference && difference.changeImpact, ResourceImpact.WILL_DESTROY, 'the difference reflects that the resource will be deleted'); test.done(); }, @@ -100,7 +100,7 @@ exports.diffTemplate = { const difference = differences.resources.changes.BucketPolicyResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketPolicyResource logical ID'); test.ok(difference && difference.isRemoval, 'the difference reflects there is no such resource after'); - test.deepEqual(difference && difference.resourceType, 'AWS::S3::BucketPolicy', 'the difference reports the resource type'); + test.deepEqual(difference && difference.oldResourceType, 'AWS::S3::BucketPolicy', 'the difference reports the resource type'); test.equal(difference && difference.changeImpact, ResourceImpact.WILL_ORPHAN, 'the difference reflects that the resource will be orphaned'); test.done(); }, @@ -141,13 +141,42 @@ exports.diffTemplate = { test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); - test.equal(difference && difference.resourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); + test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyChanges, { BucketName: { oldValue: bucketName, newValue: newBucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); test.done(); }, + 'change in dependencies counts as a simple update'(test: Test) { + // GIVEN + const currentTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['SomeResource'] + } + } + }; + + // WHEN + const newTemplate = { + Resources: { + BucketResource: { + Type: 'AWS::S3::Bucket', + DependsOn: ['SomeResource', 'SomeOtherResource'] + } + } + }; + const differences = diffTemplate(currentTemplate, newTemplate); + + // THEN + test.equal(differences.count, 1, 'no change'); + const difference = differences.resources.changes.BucketResource; + test.equal(difference && difference.changeImpact, ResourceImpact.WILL_UPDATE, 'the difference reflects that the resource will be replaced'); + test.done(); + }, + 'when a property is deleted': (test: Test) => { const bucketName = 'ShineyBucketName'; const currentTemplate = { @@ -180,7 +209,7 @@ exports.diffTemplate = { test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); - test.equal(difference && difference.resourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); + test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyChanges, { BucketName: { oldValue: bucketName, newValue: undefined, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); @@ -219,7 +248,7 @@ exports.diffTemplate = { test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); - test.equal(difference && difference.resourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); + test.equal(difference && difference.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyChanges, { BucketName: { oldValue: undefined, newValue: bucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, 'the difference reports property-level changes'); @@ -254,9 +283,8 @@ exports.diffTemplate = { test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); const difference = differences.resources.changes.BucketResource; test.notStrictEqual(difference, undefined, 'the difference is on the BucketResource logical ID'); - test.deepEqual(difference && difference.resourceType, - { oldType: 'AWS::IAM::Policy', newType: 'AWS::S3::Bucket' }, - 'the difference reflects the type change'); + test.deepEqual(difference && difference.oldResourceType, 'AWS::IAM::Policy'); + test.deepEqual(difference && difference.newResourceType, 'AWS::S3::Bucket'); test.equal(difference && difference.changeImpact, ResourceImpact.WILL_REPLACE, 'the difference reflects that the resource will be replaced'); test.done(); },