Skip to content

Commit

Permalink
fix(cloudformation-diff): ignore changes to DependsOn (#1005)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@rix0rrr
rix0rrr authored Oct 25, 2018
1 parent db4e718 commit 3605f9c
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 37 deletions.
29 changes: 13 additions & 16 deletions packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<any> } = {};
let otherChanges: { [key: string]: types.Difference<any> } = {};

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 });
Expand Down
31 changes: 22 additions & 9 deletions packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,17 +210,18 @@ export interface Resource {
[key: string]: any;
}
export class ResourceDifference extends Difference<Resource> {
/** 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<any> };
/** Changes to non-property level attributes of the resource */
public readonly otherChanges: { [key: string]: Difference<any> };

/** 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<any> },
otherChanges: { [key: string]: Difference<any> }
}
Expand All @@ -231,14 +232,26 @@ export class ResourceDifference extends Difference<Resource> {
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);
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/cloudformation-diff/lib/format.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
}
}

Expand Down
46 changes: 37 additions & 9 deletions packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},
Expand All @@ -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();
},
Expand Down Expand Up @@ -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();
},
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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();
},
Expand Down

0 comments on commit 3605f9c

Please sign in to comment.