From 3270b4727c05156e43101933e22b0b94ca3fdaaf Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 16 Jan 2019 11:30:02 +0100 Subject: [PATCH] fix(aws-cdk): Improvements to IAM diff rendering (#1542) Contains the following changes: - Print table to STDOUT instead of STDERR. This ensures the diff output and the "confirm (y/n)" prompt that follows it don't interleave on the terminal, degrading readability. - Control table width to not exceed the terminal width, so that it doesn't wrap and lead to a mess of lines. - Switch back to the new version of the `table` library (instead of `cli-table`) which now has in-cell newline support, to get rid of the rendering bugs in `cli-table` that would occasionally eat newlines. - Render resource `replace` impact as red instead of yellow, to indicate that data loss will be happening here (fixes #1458). - Change replacement diffing in order to not trigger IAM changes dialog (fixes #1495). - Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549). Explanation: We used to modify the new target template in place, causing changes to the logical ids of replaced resources, which would trigger downstream changes, which would further trigger potential downstream replacements, etc. This would properly calculate the set of impacted resources, but would also lead to the modified 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. --- .../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 | 216 +++++++++++++----- .../cloudformation-diff/lib/diff/util.ts | 1 - .../cloudformation-diff/lib/format-table.ts | 111 +++++++++ .../cloudformation-diff/lib/format.ts | 65 ++---- .../@aws-cdk/cloudformation-diff/lib/index.ts | 1 + .../@aws-cdk/cloudformation-diff/package.json | 6 +- .../test/test.diff-template.ts | 40 ++-- packages/aws-cdk/integ-tests/test-cdk-ls.sh | 4 +- packages/aws-cdk/lib/commands/context.ts | 12 +- packages/aws-cdk/lib/diff.ts | 4 +- packages/aws-cdk/lib/util/tables.ts | 18 +- packages/aws-cdk/package.json | 5 +- 15 files changed, 395 insertions(+), 189 deletions(-) create mode 100644 packages/@aws-cdk/cloudformation-diff/lib/format-table.ts 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..a7cc6d703de2e 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 changes(): { [logicalId: string]: T } { + return onlyChanges(this.diffs); + } + + public get differenceCount(): number { + return Object.values(this.changes).length; + } - public get count(): number { - return this.logicalIds.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,32 @@ export class ResourceDifference extends Difference { return ResourceImpact.WILL_REPLACE; } - return Object.values(this.propertyUpdates) + // 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.WILL_UPDATE); + .reduce(worstImpact, baseImpact); } - 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 +643,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-table.ts b/packages/@aws-cdk/cloudformation-diff/lib/format-table.ts new file mode 100644 index 0000000000000..cd9a05c4b478f --- /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(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]); + } + }).trimRight(); +} + +/** + * 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: 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 + 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): 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 + const contentWidth = terminalWidth - 2 - columns.length * 3; + + // If we don't exceed the terminal width, do nothing + if (sum(columns) <= contentWidth) { return columns; } + + 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..fcf6c35a2b162 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'; @@ -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); }); } } @@ -186,13 +189,14 @@ 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: 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 } } @@ -352,12 +356,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 +369,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 +386,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/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/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 5c1c1454d720b..c3789f258552d 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.1", "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" 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/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" < { 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/diff.ts b/packages/aws-cdk/lib/diff.ts index 5a56a3fc6b1a0..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 { @@ -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; diff --git a/packages/aws-cdk/lib/util/tables.ts b/packages/aws-cdk/lib/util/tables.ts index 56a42f13fb105..bfc059bef40fc 100644 --- a/packages/aws-cdk/lib/util/tables.ts +++ b/packages/aws-cdk/lib/util/tables.ts @@ -1,13 +1,7 @@ -import Table = require('cli-table'); +import cfnDiff = require('@aws-cdk/cloudformation-diff'); -export interface RenderTableOptions { - colWidths?: number[]; -} - -export function renderTable(cells: string[][], options: RenderTableOptions = {}): string { - const head = cells.splice(0, 1)[0]; - - const table = new Table({ head, style: { head: [] }, colWidths: options.colWidths }); - table.push(...cells); - return table.toString(); -} +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 diff --git a/packages/aws-cdk/package.json b/packages/aws-cdk/package.json index d88f4e79ca779..f67843cc6c278 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.1", "yaml": "^1.1.0", "yargs": "^9.0.1" },