Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cloudformation-diff): ignore changes to DependsOn #1005

Merged
merged 2 commits into from
Oct 25, 2018
Merged

fix(cloudformation-diff): ignore changes to DependsOn #1005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c47cfd
fix(cloudformation-diff): ignore changes to DependsOn
Oct 24, 2018
d13fdf6
Update references to renamed properties
Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 @@ -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 = {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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();
}
Expand Down