From 7c47cfdaac650f75f69a1fe23e64a82a8f8b6710 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 24 Oct 2018 12:40:21 +0200 Subject: [PATCH] fix(cloudformation-diff): ignore changes to DependsOn 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 +++++++++---- .../test/test.diff-template.ts | 46 +++++++++++++++---- 3 files changed, 72 insertions(+), 34 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/test/test.diff-template.ts b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts index d3eab88f42260..dd6522952e7c4 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(); }, @@ -137,13 +137,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 = { @@ -172,7 +201,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'); @@ -207,7 +236,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'); @@ -242,9 +271,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(); }