From 07264c4af8ca6f11c1fb7af348c7fa3458bc1088 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 11 Jan 2019 13:49:54 +0100 Subject: [PATCH 01/10] Print diff to stdout just like the confirmation prompt This ensure so that the diff output and the prompt don't interleave. --- packages/aws-cdk/lib/diff.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 5a56a3fc6b1a0..ad14e47da4de0 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -61,7 +61,7 @@ export function printSecurityDiff(oldTemplate: any, newTemplate: cxapi.Synthesiz warning(`This deployment will make potentially sensitive changes according to your current security approval level (--require-approval ${requireApproval}).`); warning(`Please confirm you intend to make the following modifications:\n`); - cfnDiff.formatSecurityChanges(process.stderr, diff, buildLogicalToPathMap(newTemplate)); + cfnDiff.formatSecurityChanges(process.stdout, diff, buildLogicalToPathMap(newTemplate)); return true; } return false; From f0975c191e91e95ddd23c5b47a572158e25aa536 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 11:31:55 +0100 Subject: [PATCH 02/10] Fix table rendering Calculcate column widths so that the table will not overflow the terminal anymore. Switch back to 'table' library which now has newline support and not the rendering bug that 'cli-table' had. --- .../cloudformation-diff/lib/format-table.ts | 111 ++++++++++++++++++ .../cloudformation-diff/lib/format.ts | 49 +------- .../@aws-cdk/cloudformation-diff/package.json | 6 +- 3 files changed, 119 insertions(+), 47 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/format-table.ts diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts new file mode 100644 index 0000000000000..31632f3a9f0cc --- /dev/null +++ b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts @@ -0,0 +1,111 @@ +import colors = require('colors/safe'); +import stringWidth = require('string-width'); +import table = require('table'); + +/** + * Render a two-dimensional array to a visually attractive table + * + * First row is considered the table header. + */ +export function formatTable(cells: string[][], columns: number | undefined): string { + return table.table(cells, { + border: TABLE_BORDER_CHARACTERS, + columns: buildColumnConfig(calculcateColumnWidths(cells, columns)), + drawHorizontalLine: (line) => { + // Numbering like this: [line 0] [header = row[0]] [line 1] [row 1] [line 2] [content 2] [line 3] + return (line < 2 || line === cells.length) || lineBetween(cells[line - 1], cells[line]); + } + }); +} + +/** + * Whether we should draw a line between two rows + * + * Draw horizontal line if 2nd column values are different. + */ +function lineBetween(rowA: string[], rowB: string[]) { + return rowA[1] !== rowB[1]; +} + +function buildColumnConfig(widths: Array): { [index: number]: table.ColumnConfig } { + const ret: { [index: number]: table.ColumnConfig } = {}; + widths.forEach((width, i) => { + ret[i] = { width, useWordWrap: true } as any; // 'useWordWrap' is not in @types/table + if (width === undefined) { + delete ret[i].width; + } + }); + + return ret; +} + +/** + * Calculate column widths given a terminal width + * + * We do this by calculating a fair share for every column. Extra width smaller + * than the fair share is evenly distributed over all columns that exceed their + * fair share. + */ +function calculcateColumnWidths(rows: string[][], terminalWidth: number | undefined): Array { + // use 'string-width' to not count ANSI chars as actual character width + const columns = rows[0].map((_, i) => Math.max(...rows.map(row => stringWidth(row[i])))); + + // If we have no terminal width, do nothing + if (terminalWidth === undefined) { return columns.map(_ => undefined); } + + const contentWidth = terminalWidth - 2 - columns.length * 3; + + // If we don't exceed the terminal width, do nothing + if (sum(columns) <= contentWidth) { return columns.map(_ => undefined); } + + const fairShare = Math.min(contentWidth / columns.length); + const smallColumns = columns.filter(w => w < fairShare); + + let distributableWidth = contentWidth - sum(smallColumns); + const fairDistributable = Math.floor(distributableWidth / (columns.length - smallColumns.length)); + + const ret = new Array(); + for (const requestedWidth of columns) { + if (requestedWidth < fairShare) { + // Small column gets what they want + ret.push(requestedWidth); + } else { + // Last column gets all remaining, otherwise get fair redist share + const width = distributableWidth < 2 * fairDistributable ? distributableWidth : fairDistributable; + ret.push(width); + distributableWidth -= width; + } + } + + return ret; +} + +function sum(xs: number[]): number { + let total = 0; + for (const x of xs) { + total += x; + } + return total; +} + +// What color the table is going to be +const tableColor = colors.gray; + +// Unicode table characters with a color +const TABLE_BORDER_CHARACTERS = { + topBody: tableColor('─'), + topJoin: tableColor('┬'), + topLeft: tableColor('┌'), + topRight: tableColor('┐'), + bottomBody: tableColor('─'), + bottomJoin: tableColor('┴'), + bottomLeft: tableColor('└'), + bottomRight: tableColor('┘'), + bodyLeft: tableColor('│'), + bodyRight: tableColor('│'), + bodyJoin: tableColor('│'), + joinBody: tableColor('─'), + joinLeft: tableColor('├'), + joinRight: tableColor('┤'), + joinJoin: tableColor('┼') +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index c808dbc822497..c53265eac2a9e 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -1,10 +1,10 @@ import cxapi = require('@aws-cdk/cx-api'); -import Table = require('cli-table'); import colors = require('colors/safe'); import { format } from 'util'; import { Difference, isPropertyDifference, ResourceDifference, ResourceImpact } from './diff-template'; import { DifferenceCollection, TemplateDiff } from './diff/types'; import { deepEqual } from './diff/util'; +import { formatTable } from './format-table'; import { IamChanges } from './iam/iam-changes'; import { SecurityGroupChanges } from './network/security-group-changes'; @@ -352,12 +352,12 @@ class Formatter { if (changes.statements.hasChanges) { this.printSectionHeader('IAM Statement Changes'); - this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarizeStatements()))); + this.print(formatTable(this.deepSubstituteBracedLogicalIds(changes.summarizeStatements()), this.stream.columns)); } if (changes.managedPolicies.hasChanges) { this.printSectionHeader('IAM Policy Changes'); - this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()))); + this.print(formatTable(this.deepSubstituteBracedLogicalIds(changes.summarizeManagedPolicies()), this.stream.columns)); } } @@ -365,7 +365,7 @@ class Formatter { if (!changes.hasChanges) { return; } this.printSectionHeader('Security Group Changes'); - this.print(renderTable(this.deepSubstituteBracedLogicalIds(changes.summarize()))); + this.print(formatTable(this.deepSubstituteBracedLogicalIds(changes.summarize()), this.stream.columns)); } public deepSubstituteBracedLogicalIds(rows: string[][]): string[][] { @@ -382,47 +382,6 @@ class Formatter { } } -/** - * Render a two-dimensional array to a visually attractive table - * - * First row is considered the table header. - */ -function renderTable(cells: string[][]): string { - const head = cells.splice(0, 1)[0]; - - const table = new Table({ head, style: { head: [] } }); - table.push(...cells); - return stripHorizontalLines(table.toString()).trimRight(); -} - -/** - * Strip horizontal lines in the table rendering if the second-column values are the same - * - * We couldn't find a table library that BOTH does newlines-in-cells correctly AND - * has an option to enable/disable separator lines on a per-row basis. So we're - * going to do some character post-processing on the table instead. - */ -function stripHorizontalLines(tableRendering: string) { - const lines = tableRendering.split('\n'); - - let i = 3; - while (i < lines.length - 3) { - if (secondColumnValue(lines[i]) === secondColumnValue(lines[i + 2])) { - lines.splice(i + 1, 1); - i += 1; - } else { - i += 2; - } - } - - return lines.join('\n'); - - function secondColumnValue(line: string) { - const cols = colors.stripColors(line).split('│').filter(x => x !== ''); - return cols[1]; - } -} - /** * A patch as returned by ``diff.structuredPatch``. */ diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 5c1c1454d720b..4bf691c47fa5a 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -25,14 +25,16 @@ "dependencies": { "@aws-cdk/cfnspec": "^0.22.0", "@aws-cdk/cx-api": "^0.22.0", - "cli-table": "^0.3.1", + "string-width": "^2.1.1", + "table": "^5.2.0", "colors": "^1.2.1", "diff": "^4.0.1", "fast-deep-equal": "^2.0.1", "source-map-support": "^0.5.6" }, "devDependencies": { - "@types/cli-table": "^0.3.0", + "@types/table": "^4.0.5", + "@types/string-width": "^2.0.0", "cdk-build-tools": "^0.22.0", "fast-check": "^1.8.0", "pkglint": "^0.22.0" From e4fec197c705555a2694206d5b5299e4d51f0529 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 13:33:14 +0100 Subject: [PATCH 03/10] Format 'replace' as red as well since it's just as dangerous as 'destroy'. Fixes #1458. --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index c53265eac2a9e..e37703d0da059 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -186,7 +186,7 @@ class Formatter { case ResourceImpact.MAY_REPLACE: return colors.italic(colors.yellow('may be replaced')); case ResourceImpact.WILL_REPLACE: - return colors.italic(colors.bold(colors.yellow('replace'))); + return colors.italic(colors.bold(colors.red('replace'))); case ResourceImpact.WILL_DESTROY: return colors.italic(colors.bold(colors.red('destroy'))); case ResourceImpact.WILL_ORPHAN: From 51f805272a37bb1988ae3f23f238dd681f4d4c52 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 13:37:12 +0100 Subject: [PATCH 04/10] Change replacement diffing to not trigger IAM changes dialog We used to modify the new template in place, causing changes to "logicalIDs" of replaced resources, which would trigger downstream changes and potential downstream replacements, etc. This would lead to the changed logical ID popping up in the IAM diff which was not desirable. In this change, we do the same processing but we throw away the template after all changes have been propagated, and only copy the resultant property STATUSES onto the diff object that gets returned to the user. This leads to the same displayed result without the changes to the template actually propagating. In the course of this modification, the diff classes have been changed to also have objects in places of resources and properties that didn't actually change (so that they could be modified in-place), and diff objects have a boolean telling whether they are actual changes or not. Fixes #1495. --- .../assert/lib/assertions/match-template.ts | 2 +- .../cloudformation-diff/lib/diff-template.ts | 79 +++++-- .../cloudformation-diff/lib/diff/index.ts | 20 +- .../cloudformation-diff/lib/diff/types.ts | 210 +++++++++++++----- .../cloudformation-diff/lib/diff/util.ts | 1 - .../cloudformation-diff/lib/format.ts | 14 +- .../test/test.diff-template.ts | 40 ++-- packages/aws-cdk/lib/diff.ts | 2 +- 8 files changed, 246 insertions(+), 122 deletions(-) diff --git a/packages/@aws-cdk/assert/lib/assertions/match-template.ts b/packages/@aws-cdk/assert/lib/assertions/match-template.ts index 7ec358081fdb2..5e7f3d5d0556c 100644 --- a/packages/@aws-cdk/assert/lib/assertions/match-template.ts +++ b/packages/@aws-cdk/assert/lib/assertions/match-template.ts @@ -55,7 +55,7 @@ class StackMatchesTemplateAssertion extends Assertion { private isDiffAcceptable(diff: cfnDiff.TemplateDiff): boolean { switch (this.matchStyle) { case MatchStyle.EXACT: - return diff.count === 0; + return diff.differenceCount === 0; case MatchStyle.NO_REPLACES: for (const key of Object.keys(diff.resources.changes)) { const change = diff.resources.changes[key]!; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index b9d47e7080456..f10bb0a1b3885 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -39,42 +39,77 @@ const DIFF_HANDLERS: HandlerRegistry = { * the template +newTemplate+. */ export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff { + // Base diff + const theDiff = calculateTemplateDiff(currentTemplate, newTemplate); + // We're going to modify this in-place - newTemplate = deepCopy(newTemplate); - - while (true) { - const differences: types.ITemplateDiff = {}; - const unknown: { [key: string]: types.Difference } = {}; - for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) { - const oldValue = currentTemplate[key]; - const newValue = newTemplate[key]; - if (deepEqual(oldValue, newValue)) { continue; } - const handler: DiffHandler = DIFF_HANDLERS[key] - || ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV)); - handler(differences, oldValue, newValue); + const newTemplateCopy = deepCopy(newTemplate); - } - if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); } + let didPropagateReferenceChanges; + let diffWithReplacements; + do { + diffWithReplacements = calculateTemplateDiff(currentTemplate, newTemplateCopy); // Propagate replacements for replaced resources - let didPropagateReferenceChanges = false; - if (differences.resources) { - differences.resources.forEach((logicalId, change) => { + didPropagateReferenceChanges = false; + if (diffWithReplacements.resources) { + diffWithReplacements.resources.forEachDifference((logicalId, change) => { if (change.changeImpact === types.ResourceImpact.WILL_REPLACE) { - if (propagateReplacedReferences(newTemplate, logicalId)) { + if (propagateReplacedReferences(newTemplateCopy, logicalId)) { didPropagateReferenceChanges = true; } } }); } + } while (didPropagateReferenceChanges); + + // Copy "replaced" states from `diffWithReplacements` to `theDiff`. + diffWithReplacements.resources + .filter(r => isReplacement(r!.changeImpact)) + .forEachDifference((logicalId, downstreamReplacement) => { + const resource = theDiff.resources.get(logicalId); + + if (resource.changeImpact !== downstreamReplacement.changeImpact) { + propagatePropertyReplacement(downstreamReplacement, resource); + } + }); + + return theDiff; +} + +function isReplacement(impact: types.ResourceImpact) { + return impact === types.ResourceImpact.MAY_REPLACE || impact === types.ResourceImpact.WILL_REPLACE; +} - // We're done only if we didn't have to propagate any more replacements. - if (!didPropagateReferenceChanges) { - return new types.TemplateDiff(differences); +/** + * For all properties in 'source' that have a "replacement" impact, propagate that impact to "dest" + */ +function propagatePropertyReplacement(source: types.ResourceDifference, dest: types.ResourceDifference) { + for (const [propertyName, diff] of Object.entries(source.propertyUpdates)) { + if (diff.changeImpact && isReplacement(diff.changeImpact)) { + // Use the propertydiff of source in target. The result of this happens to be clear enough. + dest.setPropertyChange(propertyName, diff); } } } +function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff { + const differences: types.ITemplateDiff = {}; + const unknown: { [key: string]: types.Difference } = {}; + for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) { + const oldValue = currentTemplate[key]; + const newValue = newTemplate[key]; + if (deepEqual(oldValue, newValue)) { continue; } + const handler: DiffHandler = DIFF_HANDLERS[key] + || ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV)); + handler(differences, oldValue, newValue); + + } + if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); } + + return new types.TemplateDiff(differences); +} + /** * Compare two CloudFormation resources and return semantic differences between them */ @@ -109,7 +144,7 @@ function propagateReplacedReferences(template: object, logicalId: string): boole if (key === 'Ref') { if (obj.Ref === logicalId) { - obj.Ref = logicalId + '(replaced)'; + obj.Ref = logicalId + ' (replaced)'; ret = true; } return true; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts index 0add877280471..1bcfd6d470970 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/index.ts @@ -1,6 +1,6 @@ import cfnspec = require('@aws-cdk/cfnspec'); import types = require('./types'); -import { diffKeyedEntities } from './util'; +import { deepEqual, diffKeyedEntities } from './util'; export function diffAttribute(oldValue: any, newValue: any): types.Difference { return new types.Difference(_asString(oldValue), _asString(newValue)); @@ -31,31 +31,29 @@ export function diffResource(oldValue?: types.Resource, newValue?: types.Resourc oldType: oldValue && oldValue.Type, newType: newValue && newValue.Type }; - let propertyUpdates: { [key: string]: types.PropertyDifference } = {}; - let otherChanges: { [key: string]: types.Difference } = {}; + let propertyDiffs: { [key: string]: types.PropertyDifference } = {}; + let otherDiffs: { [key: string]: types.Difference } = {}; if (resourceType.oldType !== undefined && 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]; - propertyUpdates = diffKeyedEntities(oldValue!.Properties, + propertyDiffs = diffKeyedEntities(oldValue!.Properties, newValue!.Properties, (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl)); - otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther); - delete otherChanges.Properties; + otherDiffs = diffKeyedEntities(oldValue, newValue, _diffOther); + delete otherDiffs.Properties; } return new types.ResourceDifference(oldValue, newValue, { - resourceType, propertyUpdates, otherChanges, - oldProperties: oldValue && oldValue.Properties, - newProperties: newValue && newValue.Properties, + resourceType, propertyDiffs, otherDiffs, }); function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: cfnspec.schema.ResourceType) { - let changeImpact; + let changeImpact = types.ResourceImpact.NO_CHANGE; const spec = resourceSpec && resourceSpec.Properties && resourceSpec.Properties[key]; - if (spec) { + if (spec && !deepEqual(oldV, newV)) { switch (spec.UpdateType) { case 'Immutable': changeImpact = types.ResourceImpact.WILL_REPLACE; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index f222e56171ac3..3294b9c76b067 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -62,7 +62,7 @@ export class TemplateDiff implements ITemplateDiff { }); } - public get count() { + public get differenceCount() { let count = 0; if (this.awsTemplateFormatVersion !== undefined) { @@ -75,19 +75,19 @@ export class TemplateDiff implements ITemplateDiff { count += 1; } - count += this.conditions.count; - count += this.mappings.count; - count += this.metadata.count; - count += this.outputs.count; - count += this.parameters.count; - count += this.resources.count; - count += this.unknown.count; + count += this.conditions.differenceCount; + count += this.mappings.differenceCount; + count += this.metadata.differenceCount; + count += this.outputs.differenceCount; + count += this.parameters.differenceCount; + count += this.resources.differenceCount; + count += this.unknown.differenceCount; return count; } public get isEmpty(): boolean { - return this.count === 0; + return this.differenceCount === 0; } /** @@ -257,10 +257,24 @@ export interface ResourceChange { newProperties?: PropertyMap; } +export interface IDifference { + readonly oldValue: ValueType | undefined; + readonly newValue: ValueType | undefined; + readonly isDifferent: boolean; + readonly isAddition: boolean; + readonly isRemoval: boolean; + readonly isUpdate: boolean; +} + /** * Models an entity that changed between two versions of a CloudFormation template. */ -export class Difference { +export class Difference implements IDifference { + /** + * Whether this is an actual different or the values are actually the same + */ + public readonly isDifferent: boolean; + /** * @param oldValue the old value, cannot be equal (to the sense of +deepEqual+) to +newValue+. * @param newValue the new value, cannot be equal (to the sense of +deepEqual+) to +oldValue+. @@ -269,11 +283,7 @@ export class Difference { if (oldValue === undefined && newValue === undefined) { throw new AssertionError({ message: 'oldValue and newValue are both undefined!' }); } - if (deepEqual(oldValue, newValue)) { - const oldStr = JSON.stringify(oldValue); - const newStr = JSON.stringify(newValue); - throw new NoDifferenceError(`oldValue (${oldStr}) and newValue (${newStr}) are equal!`); - } + this.isDifferent = !deepEqual(oldValue, newValue); } /** @returns +true+ if the element is new to the template. */ @@ -302,11 +312,21 @@ export class PropertyDifference extends Difference { } } -export class DifferenceCollection> { - constructor(public readonly changes: { [logicalId: string]: T | undefined }) {} +export class DifferenceCollection> { + constructor(private readonly diffs: { [logicalId: string]: T }) {} - public get count(): number { - return this.logicalIds.length; + public get changes(): { [logicalId: string]: T } { + return onlyChanges(this.diffs); + } + + public get differenceCount(): number { + return Object.values(this.changes).length; + } + + public get(logicalId: string): T { + const ret = this.diffs[logicalId]; + if (!ret) { throw new Error(`No object with logical ID '${logicalId}'`); } + return ret; } public get logicalIds(): string[] { @@ -318,7 +338,7 @@ export class DifferenceCollection> { * returns `true`. */ public filter(predicate: (diff: T | undefined) => boolean): DifferenceCollection { - const newChanges: { [logicalId: string]: T | undefined } = { }; + const newChanges: { [logicalId: string]: T } = { }; for (const id of Object.keys(this.changes)) { const diff = this.changes[id]; @@ -341,7 +361,7 @@ export class DifferenceCollection> { * * @param cb */ - public forEach(cb: (logicalId: string, change: T) => any): void { + public forEachDifference(cb: (logicalId: string, change: T) => any): void { const removed = new Array<{ logicalId: string, change: T }>(); const added = new Array<{ logicalId: string, change: T }>(); const updated = new Array<{ logicalId: string, change: T }>(); @@ -355,7 +375,7 @@ export class DifferenceCollection> { removed.push({ logicalId, change }); } else if (change.isUpdate) { updated.push({ logicalId, change }); - } else { + } else if (change.isDifferent) { others.push({ logicalId, change }); } } @@ -372,9 +392,9 @@ export class DifferenceCollection> { * of (relative) conciseness of the constructor's signature. */ export interface ITemplateDiff { - awsTemplateFormatVersion?: Difference; - description?: Difference; - transform?: Difference; + awsTemplateFormatVersion?: IDifference; + description?: IDifference; + transform?: IDifference; conditions?: DifferenceCollection; mappings?: DifferenceCollection; @@ -383,7 +403,7 @@ export interface ITemplateDiff { parameters?: DifferenceCollection; resources?: DifferenceCollection; - unknown?: DifferenceCollection>; + unknown?: DifferenceCollection>; } export type Condition = any; @@ -423,7 +443,9 @@ export enum ResourceImpact { /** The existing physical resource will be destroyed */ WILL_DESTROY = 'WILL_DESTROY', /** The existing physical resource will be removed from CloudFormation supervision */ - WILL_ORPHAN = 'WILL_ORPHAN' + WILL_ORPHAN = 'WILL_ORPHAN', + /** There is no change in this resource */ + NO_CHANGE = 'NO_CHANGE', } /** @@ -436,12 +458,13 @@ export enum ResourceImpact { function worstImpact(one: ResourceImpact, two?: ResourceImpact): ResourceImpact { if (!two) { return one; } const badness = { - [ResourceImpact.WILL_UPDATE]: 0, - [ResourceImpact.WILL_CREATE]: 1, - [ResourceImpact.WILL_ORPHAN]: 2, - [ResourceImpact.MAY_REPLACE]: 3, - [ResourceImpact.WILL_REPLACE]: 4, - [ResourceImpact.WILL_DESTROY]: 5, + [ResourceImpact.NO_CHANGE]: 0, + [ResourceImpact.WILL_UPDATE]: 1, + [ResourceImpact.WILL_CREATE]: 2, + [ResourceImpact.WILL_ORPHAN]: 3, + [ResourceImpact.MAY_REPLACE]: 4, + [ResourceImpact.WILL_REPLACE]: 5, + [ResourceImpact.WILL_DESTROY]: 6, }; return badness[one] > badness[two] ? one : two; } @@ -453,41 +476,67 @@ export interface Resource { [key: string]: any; } -export class ResourceDifference extends Difference { +/** + * Change to a single resource between two CloudFormation templates + * + * This class can be mutated after construction. + */ +export class ResourceDifference implements IDifference { /** - * Old property values + * Whether this resource was added */ - public readonly oldProperties?: PropertyMap; + public readonly isAddition: boolean; /** - * New property values + * Whether this resource was removed */ - public readonly newProperties?: PropertyMap; + public readonly isRemoval: boolean; /** Property-level changes on the resource */ - public readonly propertyUpdates: { [key: string]: PropertyDifference }; + private readonly propertyDiffs: { [key: string]: PropertyDifference }; + /** Changes to non-property level attributes of the resource */ - public readonly otherChanges: { [key: string]: Difference }; + private readonly otherDiffs: { [key: string]: Difference }; /** The resource type (or old and new type if it has changed) */ private readonly resourceTypes: { readonly oldType?: string, readonly newType?: string }; - constructor(oldValue: Resource | undefined, - newValue: Resource | undefined, + constructor(public readonly oldValue: Resource | undefined, + public readonly newValue: Resource | undefined, args: { resourceType: { oldType?: string, newType?: string }, - oldProperties?: PropertyMap, - newProperties?: PropertyMap, - propertyUpdates: { [key: string]: PropertyDifference }, - otherChanges: { [key: string]: Difference } + propertyDiffs: { [key: string]: PropertyDifference }, + otherDiffs: { [key: string]: Difference } } ) { - super(oldValue, newValue); this.resourceTypes = args.resourceType; - this.propertyUpdates = args.propertyUpdates; - this.otherChanges = args.otherChanges; - this.oldProperties = args.oldProperties; - this.newProperties = args.newProperties; + this.propertyDiffs = args.propertyDiffs; + this.otherDiffs = args.otherDiffs; + + this.isAddition = oldValue === undefined; + this.isRemoval = newValue === undefined; + } + + public get oldProperties(): PropertyMap | undefined { + return this.oldValue && this.oldValue.Properties; + } + + public get newProperties(): PropertyMap | undefined { + return this.newValue && this.newValue.Properties; + } + + /** + * Whether this resource was modified at all + */ + public get isDifferent(): boolean { + return this.differenceCount > 0 || this.oldResourceType !== this.newResourceType; + } + + /** + * Whether the resource was updated in-place + */ + public get isUpdate(): boolean { + return this.isDifferent && !this.isAddition && !this.isRemoval; } public get oldResourceType(): string | undefined { @@ -498,6 +547,20 @@ export class ResourceDifference extends Difference { return this.resourceTypes.newType; } + /** + * All actual property updates + */ + public get propertyUpdates(): { [key: string]: PropertyDifference } { + return onlyChanges(this.propertyDiffs); + } + + /** + * All actual "other" updates + */ + public get otherChanges(): { [key: string]: Difference } { + return onlyChanges(this.otherDiffs); + } + /** * Return whether the resource type was changed in this diff * @@ -522,6 +585,18 @@ export class ResourceDifference extends Difference { return this.resourceTypes.oldType || this.resourceTypes.newType!; } + /** + * Replace a PropertyChange in this object + * + * This affects the property diff as it is summarized to users, but it DOES + * NOT affect either the "oldValue" or "newValue" values; those still contain + * the actual template values as provided by the user (they might still be + * used for downstream processing). + */ + public setPropertyChange(propertyName: string, change: PropertyDifference) { + this.propertyDiffs[propertyName] = change; + } + public get changeImpact(): ResourceImpact { // Check the Type first if (this.resourceTypes.oldType !== this.resourceTypes.newType) { @@ -534,22 +609,28 @@ export class ResourceDifference extends Difference { return ResourceImpact.WILL_REPLACE; } - return Object.values(this.propertyUpdates) + return Object.values(this.propertyDiffs) .map(elt => elt.changeImpact) .reduce(worstImpact, ResourceImpact.WILL_UPDATE); } - public get count(): number { - return Object.keys(this.propertyUpdates).length - + Object.keys(this.otherChanges).length; + /** + * Count of actual differences (not of elements) + */ + public get differenceCount(): number { + return Object.values(this.propertyUpdates).length + + Object.values(this.otherChanges).length; } - public forEach(cb: (type: 'Property' | 'Other', name: string, value: Difference | PropertyDifference) => any) { + /** + * Invoke a callback for each actual difference + */ + public forEachDifference(cb: (type: 'Property' | 'Other', name: string, value: Difference | PropertyDifference) => any) { for (const key of Object.keys(this.propertyUpdates).sort()) { cb('Property', key, this.propertyUpdates[key]); } for (const key of Object.keys(this.otherChanges).sort()) { - cb('Other', key, this.otherChanges[key]); + cb('Other', key, this.otherDiffs[key]); } } } @@ -558,8 +639,15 @@ export function isPropertyDifference(diff: Difference): diff is PropertyDi return (diff as PropertyDifference).changeImpact !== undefined; } -class NoDifferenceError extends Error { - constructor(message: string) { - super(`No difference: ${message}`); +/** + * Filter a map of IDifferences down to only retain the actual changes + */ +function onlyChanges>(xs: {[key: string]: T}): {[key: string]: T} { + const ret: { [key: string]: T } = {}; + for (const [key, diff] of Object.entries(xs)) { + if (diff.isDifferent) { + ret[key] = diff; + } } -} + return ret; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts index e0832f2bf8ff6..badd8e9ac3f48 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/util.ts @@ -54,7 +54,6 @@ export function diffKeyedEntities(oldValue: { [key: string]: any } | undefine for (const logicalId of unionOf(Object.keys(oldValue || {}), Object.keys(newValue || {}))) { const oldElement = oldValue && oldValue[logicalId]; const newElement = newValue && newValue[logicalId]; - if (deepEqual(oldElement, newElement)) { continue; } result[logicalId] = elementDiff(oldElement, newElement, logicalId); } return result; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index e37703d0da059..fcf6c35a2b162 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -96,12 +96,12 @@ class Formatter { collection: DifferenceCollection, formatter: (type: string, id: string, diff: T) => void = this.formatDifference.bind(this)) { - if (collection.count === 0) { + if (collection.differenceCount === 0) { return; } this.printSectionHeader(title); - collection.forEach((id, diff) => formatter(entryType, id, diff)); + collection.forEachDifference((id, diff) => formatter(entryType, id, diff)); this.printSectionFooter(); } @@ -120,7 +120,7 @@ class Formatter { * @param diff the difference to be rendered. */ public formatDifference(type: string, logicalId: string, diff: Difference | undefined) { - if (!diff) { return; } + if (!diff || !diff.isDifferent) { return; } let value; @@ -144,16 +144,19 @@ class Formatter { * @param diff the change to be rendered. */ public formatResourceDifference(_type: string, logicalId: string, diff: ResourceDifference) { + if (!diff.isDifferent) { return; } + const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType; // tslint:disable-next-line:max-line-length this.print(`${this.formatPrefix(diff)} ${this.formatValue(resourceType, colors.cyan)} ${this.formatLogicalId(logicalId)} ${this.formatImpact(diff.changeImpact)}`); if (diff.isUpdate) { + const differenceCount = diff.differenceCount; let processedCount = 0; - diff.forEach((_, name, values) => { + diff.forEachDifference((_, name, values) => { processedCount += 1; - this.formatTreeDiff(name, values, processedCount === diff.count); + this.formatTreeDiff(name, values, processedCount === differenceCount); }); } } @@ -193,6 +196,7 @@ class Formatter { return colors.italic(colors.yellow('orphan')); case ResourceImpact.WILL_UPDATE: case ResourceImpact.WILL_CREATE: + case ResourceImpact.NO_CHANGE: return ''; // no extra info is gained here } } 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 01cdc3663dcaf..3b490bb0a95c6 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts @@ -30,7 +30,7 @@ exports.diffTemplate = { const newTemplate = JSON.parse(JSON.stringify(currentTemplate)); const differences = diffTemplate(currentTemplate, newTemplate); - test.deepEqual(differences.count, 0, 'returns an empty diff'); + test.deepEqual(differences.differenceCount, 0, 'returns an empty diff'); test.done(); }, @@ -40,8 +40,8 @@ exports.diffTemplate = { const newTemplate = { Resources: { BucketResource: { Type: 'AWS::S3::Bucket' } } }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 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.ok(difference && difference.isAddition, 'the difference reflects there was no such resource before'); @@ -65,8 +65,8 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 1, 'the difference is in the Resources section'); 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'); @@ -95,8 +95,8 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 1, 'the difference is in the Resources section'); 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'); @@ -137,13 +137,13 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 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.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyUpdates, - { BucketName: { oldValue: bucketName, newValue: newBucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, + { BucketName: { oldValue: bucketName, newValue: newBucketName, changeImpact: ResourceImpact.WILL_REPLACE, isDifferent: true } }, 'the difference reports property-level changes'); test.done(); }, @@ -171,7 +171,7 @@ exports.diffTemplate = { const differences = diffTemplate(currentTemplate, newTemplate); // THEN - test.equal(differences.count, 1, 'no change'); + test.equal(differences.differenceCount, 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(); @@ -205,13 +205,13 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 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.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyUpdates, - { BucketName: { oldValue: bucketName, newValue: undefined, changeImpact: ResourceImpact.WILL_REPLACE } }, + { BucketName: { oldValue: bucketName, newValue: undefined, changeImpact: ResourceImpact.WILL_REPLACE, isDifferent: true } }, 'the difference reports property-level changes'); test.done(); }, @@ -244,13 +244,13 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 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.oldResourceType, 'AWS::S3::Bucket', 'the difference reports the resource type'); test.deepEqual(difference && difference.propertyUpdates, - { BucketName: { oldValue: undefined, newValue: bucketName, changeImpact: ResourceImpact.WILL_REPLACE } }, + { BucketName: { oldValue: undefined, newValue: bucketName, changeImpact: ResourceImpact.WILL_REPLACE, isDifferent: true } }, 'the difference reports property-level changes'); test.done(); }, @@ -279,8 +279,8 @@ exports.diffTemplate = { }; const differences = diffTemplate(currentTemplate, newTemplate); - test.equal(differences.count, 1, 'returns a single difference'); - test.equal(differences.resources.count, 1, 'the difference is in the Resources section'); + test.equal(differences.differenceCount, 1, 'returns a single difference'); + test.equal(differences.resources.differenceCount, 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.oldResourceType, 'AWS::IAM::Policy'); @@ -332,7 +332,7 @@ exports.diffTemplate = { const differences = diffTemplate(currentTemplate, newTemplate); // THEN - test.equal(differences.resources.count, 3, 'all resources are replaced'); + test.equal(differences.resources.differenceCount, 3, 'all resources are replaced'); test.done(); }, }; diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index ad14e47da4de0..e086b88bb2480 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -37,7 +37,7 @@ export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedS print(colors.green('There were no differences')); } - return diff.count; + return diff.differenceCount; } export enum RequireApproval { From ebb7f347f1a191efb7b4c7a8df037fdfe5ccd1e9 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 14:25:03 +0100 Subject: [PATCH 05/10] Fix toolkit --- .../cloudformation-diff/lib/format-table.ts | 2 +- packages/aws-cdk/lib/util/tables.ts | 40 ++++++++++++++++--- packages/aws-cdk/package.json | 5 +-- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts index 31632f3a9f0cc..a673724e18daf 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts @@ -15,7 +15,7 @@ export function formatTable(cells: string[][], columns: number | undefined): str // Numbering like this: [line 0] [header = row[0]] [line 1] [row 1] [line 2] [content 2] [line 3] return (line < 2 || line === cells.length) || lineBetween(cells[line - 1], cells[line]); } - }); + }).trimRight(); } /** diff --git a/packages/aws-cdk/lib/util/tables.ts b/packages/aws-cdk/lib/util/tables.ts index 56a42f13fb105..b1fa15248f2fe 100644 --- a/packages/aws-cdk/lib/util/tables.ts +++ b/packages/aws-cdk/lib/util/tables.ts @@ -1,13 +1,43 @@ -import Table = require('cli-table'); +import colors = require('colors/safe'); +import table = require('table'); export interface RenderTableOptions { colWidths?: number[]; } export function renderTable(cells: string[][], options: RenderTableOptions = {}): string { - const head = cells.splice(0, 1)[0]; + const columns: {[col: number]: table.ColumnConfig} = {}; - const table = new Table({ head, style: { head: [] }, colWidths: options.colWidths }); - table.push(...cells); - return table.toString(); + if (options.colWidths) { + options.colWidths.forEach((width, i) => { + columns[i] = { width, useWordWrap: true } as any; // @types doesn't have this type + }); + } + + return table.table(cells, { + border: TABLE_BORDER_CHARACTERS, + columns, + }).trimRight(); } + +// What color the table is going to be +const tableColor = colors.gray; + +// Unicode table characters with a color +const TABLE_BORDER_CHARACTERS = { + topBody: tableColor('─'), + topJoin: tableColor('┬'), + topLeft: tableColor('┌'), + topRight: tableColor('┐'), + bottomBody: tableColor('─'), + bottomJoin: tableColor('┴'), + bottomLeft: tableColor('└'), + bottomRight: tableColor('┘'), + bodyLeft: tableColor('│'), + bodyRight: tableColor('│'), + bodyJoin: tableColor('│'), + joinBody: tableColor('─'), + joinLeft: tableColor('├'), + joinRight: tableColor('┤'), + joinJoin: tableColor('┼') +}; \ No newline at end of file diff --git a/packages/aws-cdk/package.json b/packages/aws-cdk/package.json index d88f4e79ca779..6bca9e0401c67 100644 --- a/packages/aws-cdk/package.json +++ b/packages/aws-cdk/package.json @@ -32,11 +32,11 @@ "license": "Apache-2.0", "devDependencies": { "@types/archiver": "^2.1.2", - "@types/cli-table": "^0.3.0", "@types/fs-extra": "^5.0.4", "@types/minimatch": "^3.0.3", "@types/mockery": "^1.4.29", "@types/request": "^2.47.1", + "@types/table": "^4.0.5", "@types/semver": "^5.5.0", "@types/uuid": "^3.4.3", "@types/yaml": "^1.0.0", @@ -52,7 +52,6 @@ "archiver": "^2.1.1", "aws-sdk": "^2.259.1", "camelcase": "^5.0.0", - "cli-table": "^0.3.1", "colors": "^1.2.1", "decamelize": "^2.0.0", "fs-extra": "^7.0.0", @@ -63,7 +62,7 @@ "request": "^2.83.0", "semver": "^5.5.0", "source-map-support": "^0.5.6", - "table": "^5.1.0", + "table": "^5.2.0", "yaml": "^1.1.0", "yargs": "^9.0.1" }, From cf8abc3a7c756951e4a3fc45f670a3f08ef2b7c8 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 14:34:27 +0100 Subject: [PATCH 06/10] Fix toolkit integ 'ls' test --- packages/aws-cdk/integ-tests/test-cdk-ls.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/integ-tests/test-cdk-ls.sh b/packages/aws-cdk/integ-tests/test-cdk-ls.sh index 9958052b99e74..f219eb2f42f90 100755 --- a/packages/aws-cdk/integ-tests/test-cdk-ls.sh +++ b/packages/aws-cdk/integ-tests/test-cdk-ls.sh @@ -7,9 +7,11 @@ source ${scriptdir}/common.bash setup assert "cdk ls" < Date: Mon, 14 Jan 2019 14:34:38 +0100 Subject: [PATCH 07/10] Fix base case for 'resourceImpact' --- packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 3294b9c76b067..a75d8172cc0ed 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -611,7 +611,7 @@ export class ResourceDifference implements IDifference { return Object.values(this.propertyDiffs) .map(elt => elt.changeImpact) - .reduce(worstImpact, ResourceImpact.WILL_UPDATE); + .reduce(worstImpact, ResourceImpact.NO_CHANGE); } /** From 7a78b5f08bc06bc7d0538e44b3a9037bed8d6fed Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 15:12:30 +0100 Subject: [PATCH 08/10] Fix test --- packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index a75d8172cc0ed..a7cc6d703de2e 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -609,9 +609,13 @@ export class ResourceDifference implements IDifference { return ResourceImpact.WILL_REPLACE; } + // Base impact (before we mix in the worst of the property impacts); + // WILL_UPDATE if we have "other" changes, NO_CHANGE if there are no "other" changes. + const baseImpact = Object.keys(this.otherChanges).length > 0 ? ResourceImpact.WILL_UPDATE : ResourceImpact.NO_CHANGE; + return Object.values(this.propertyDiffs) .map(elt => elt.changeImpact) - .reduce(worstImpact, ResourceImpact.NO_CHANGE); + .reduce(worstImpact, baseImpact); } /** From afe38df7b120c3dbf8ea9205f21df53965494a80 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 14 Jan 2019 15:57:26 +0100 Subject: [PATCH 09/10] Switch version of 'table' that does not filter out newlines anymore --- packages/@aws-cdk/cloudformation-diff/package.json | 2 +- packages/aws-cdk/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 4bf691c47fa5a..c3789f258552d 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -26,7 +26,7 @@ "@aws-cdk/cfnspec": "^0.22.0", "@aws-cdk/cx-api": "^0.22.0", "string-width": "^2.1.1", - "table": "^5.2.0", + "table": "^5.2.1", "colors": "^1.2.1", "diff": "^4.0.1", "fast-deep-equal": "^2.0.1", diff --git a/packages/aws-cdk/package.json b/packages/aws-cdk/package.json index 6bca9e0401c67..f67843cc6c278 100644 --- a/packages/aws-cdk/package.json +++ b/packages/aws-cdk/package.json @@ -62,7 +62,7 @@ "request": "^2.83.0", "semver": "^5.5.0", "source-map-support": "^0.5.6", - "table": "^5.2.0", + "table": "^5.2.1", "yaml": "^1.1.0", "yargs": "^9.0.1" }, From 963920e08954bc2abb2e74da438ca86247b85389 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 16 Jan 2019 11:01:57 +0100 Subject: [PATCH 10/10] Use same table formatting in CDK toolkit as CFN-diff Rewrite 'undefined' logic to make it less onerous. Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549). --- .../cloudformation-diff/lib/format-table.ts | 12 ++--- .../@aws-cdk/cloudformation-diff/lib/index.ts | 1 + packages/aws-cdk/lib/commands/context.ts | 12 ++++- packages/aws-cdk/lib/util/tables.ts | 48 +++---------------- 4 files changed, 24 insertions(+), 49 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts index a673724e18daf..cd9a05c4b478f 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts @@ -10,7 +10,7 @@ import table = require('table'); export function formatTable(cells: string[][], columns: number | undefined): string { return table.table(cells, { border: TABLE_BORDER_CHARACTERS, - columns: buildColumnConfig(calculcateColumnWidths(cells, columns)), + columns: buildColumnConfig(columns !== undefined ? calculcateColumnWidths(cells, columns) : undefined), drawHorizontalLine: (line) => { // Numbering like this: [line 0] [header = row[0]] [line 1] [row 1] [line 2] [content 2] [line 3] return (line < 2 || line === cells.length) || lineBetween(cells[line - 1], cells[line]); @@ -27,7 +27,9 @@ function lineBetween(rowA: string[], rowB: string[]) { return rowA[1] !== rowB[1]; } -function buildColumnConfig(widths: Array): { [index: number]: table.ColumnConfig } { +function buildColumnConfig(widths: number[] | undefined): { [index: number]: table.ColumnConfig } | undefined { + if (widths === undefined) { return undefined; } + const ret: { [index: number]: table.ColumnConfig } = {}; widths.forEach((width, i) => { ret[i] = { width, useWordWrap: true } as any; // 'useWordWrap' is not in @types/table @@ -46,17 +48,15 @@ function buildColumnConfig(widths: Array): { [index: number] * than the fair share is evenly distributed over all columns that exceed their * fair share. */ -function calculcateColumnWidths(rows: string[][], terminalWidth: number | undefined): Array { +function calculcateColumnWidths(rows: string[][], terminalWidth: number): number[] { // use 'string-width' to not count ANSI chars as actual character width const columns = rows[0].map((_, i) => Math.max(...rows.map(row => stringWidth(row[i])))); // If we have no terminal width, do nothing - if (terminalWidth === undefined) { return columns.map(_ => undefined); } - const contentWidth = terminalWidth - 2 - columns.length * 3; // If we don't exceed the terminal width, do nothing - if (sum(columns) <= contentWidth) { return columns.map(_ => undefined); } + if (sum(columns) <= contentWidth) { return columns; } const fairShare = Math.min(contentWidth / columns.length); const smallColumns = columns.filter(w => w < fairShare); diff --git a/packages/@aws-cdk/cloudformation-diff/lib/index.ts b/packages/@aws-cdk/cloudformation-diff/lib/index.ts index 44f7a1f8cea49..34a07c7559fb7 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/index.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/index.ts @@ -1,3 +1,4 @@ export * from './diff-template'; export * from './format'; +export * from './format-table'; export { deepEqual } from './diff/util'; diff --git a/packages/aws-cdk/lib/commands/context.ts b/packages/aws-cdk/lib/commands/context.ts index 5d496de076145..3a13786eacb3d 100644 --- a/packages/aws-cdk/lib/commands/context.ts +++ b/packages/aws-cdk/lib/commands/context.ts @@ -52,6 +52,16 @@ export async function realHandler(options: CommandOptions): Promise { function listContext(context: any) { const keys = contextKeys(context); + if (keys.length === 0) { + print(`This CDK application does not have any saved context values yet.`); + print(''); + print(`Context will automatically be saved when you synthesize CDK apps`); + print(`that use environment context information like AZ information, VPCs,`); + print(`SSM parameters, and so on.`); + + return; + } + // Print config by default const data: any[] = [[colors.green('#'), colors.green('Key'), colors.green('Value')]]; for (const [i, key] of keys) { @@ -61,7 +71,7 @@ function listContext(context: any) { print(`Context found in ${colors.blue(DEFAULTS)}:\n`); - print(renderTable(data, { colWidths: [2, 50, 50] })); + print(renderTable(data, process.stdout.columns)); // tslint:disable-next-line:max-line-length print(`Run ${colors.blue('cdk context --reset KEY_OR_NUMBER')} to remove a context key. It will be refreshed on the next CDK synthesis run.`); diff --git a/packages/aws-cdk/lib/util/tables.ts b/packages/aws-cdk/lib/util/tables.ts index b1fa15248f2fe..bfc059bef40fc 100644 --- a/packages/aws-cdk/lib/util/tables.ts +++ b/packages/aws-cdk/lib/util/tables.ts @@ -1,43 +1,7 @@ -import colors = require('colors/safe'); -import table = require('table'); +import cfnDiff = require('@aws-cdk/cloudformation-diff'); -export interface RenderTableOptions { - colWidths?: number[]; -} - -export function renderTable(cells: string[][], options: RenderTableOptions = {}): string { - const columns: {[col: number]: table.ColumnConfig} = {}; - - if (options.colWidths) { - options.colWidths.forEach((width, i) => { - columns[i] = { width, useWordWrap: true } as any; // @types doesn't have this type - }); - } - - return table.table(cells, { - border: TABLE_BORDER_CHARACTERS, - columns, - }).trimRight(); -} - -// What color the table is going to be -const tableColor = colors.gray; - -// Unicode table characters with a color -const TABLE_BORDER_CHARACTERS = { - topBody: tableColor('─'), - topJoin: tableColor('┬'), - topLeft: tableColor('┌'), - topRight: tableColor('┐'), - bottomBody: tableColor('─'), - bottomJoin: tableColor('┴'), - bottomLeft: tableColor('└'), - bottomRight: tableColor('┘'), - bodyLeft: tableColor('│'), - bodyRight: tableColor('│'), - bodyJoin: tableColor('│'), - joinBody: tableColor('─'), - joinLeft: tableColor('├'), - joinRight: tableColor('┤'), - joinJoin: tableColor('┼') -}; \ No newline at end of file +export function renderTable(cells: string[][], columns?: number) { + // The cfnDiff module has logic for terminal-width aware table + // formatting (and nice colors), let's just reuse that. + return cfnDiff.formatTable(cells, columns); +} \ No newline at end of file