From 63200c3a226d61e81a9a929ed75d86d0822d9906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 7 Jan 2019 14:19:08 +0100 Subject: [PATCH 1/4] feat(diff): Better diff of random objects Use a unified diff format to render differences in arbitrary values, making it easier to understand what is changing in possibly large JSON structures, for example. --- .../cloudformation-diff/lib/diff-template.ts | 4 +- .../cloudformation-diff/lib/format.ts | 122 ++++++++++++++++-- .../cloudformation-diff/package-lock.json | 7 +- .../@aws-cdk/cloudformation-diff/package.json | 1 + packages/aws-cdk/bin/cdk.ts | 9 +- packages/aws-cdk/lib/diff.ts | 6 +- 6 files changed, 127 insertions(+), 22 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index 9a0d6ae88039b..b9d47e7080456 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -32,7 +32,7 @@ const DIFF_HANDLERS: HandlerRegistry = { * Compare two CloudFormation templates and return semantic differences between them. * * @param currentTemplate the current state of the stack. - * @param newTemplate the target state of the stack. + * @param newTemplate the target state of the stack. * * @returns a +types.TemplateDiff+ object that represents the changes that will happen if * a stack which current state is described by +currentTemplate+ is updated with @@ -144,4 +144,4 @@ function deepCopy(x: any): any { } return x; -} \ 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 df88d075cc2f6..0c18dbcdbf405 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -8,15 +8,23 @@ import { deepEqual } from './diff/util'; import { IamChanges } from './iam/iam-changes'; import { SecurityGroupChanges } from './network/security-group-changes'; +// tslint:disable-next-line:no-var-requires +const { structuredPatch } = require('diff'); + /** * Renders template differences to the process' console. * - * @param templateDiff TemplateDiff to be rendered to the console. + * @param string The IO stream where to output the rendered diff. + * @param templateDiff TemplateDiff to be rendered to the console. * @param logicalToPathMap A map from logical ID to construct path. Useful in * case there is no aws:cdk:path metadata in the template. + * @param context the number of context lines to use in arbitrary JSON diff. */ -export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }) { - const formatter = new Formatter(stream, logicalToPathMap, templateDiff); +export function formatDifferences(stream: NodeJS.WriteStream, + templateDiff: TemplateDiff, + logicalToPathMap: { [logicalId: string]: string } = { }, + context?: number) { + const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context); if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) { formatter.printSectionHeader('Template'); @@ -40,8 +48,11 @@ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: Temp /** * Renders a diff of security changes to the given stream */ -export function formatSecurityChanges(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: {[logicalId: string]: string} = {}) { - const formatter = new Formatter(stream, logicalToPathMap, templateDiff); +export function formatSecurityChanges(stream: NodeJS.WriteStream, + templateDiff: TemplateDiff, + logicalToPathMap: {[logicalId: string]: string} = {}, + context?: number) { + const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context); formatSecurityChangesWithBanner(formatter, templateDiff); } @@ -56,11 +67,15 @@ function formatSecurityChangesWithBanner(formatter: Formatter, templateDiff: Tem } const ADDITION = colors.green('[+]'); +const CONTEXT = colors.grey('[ ]'); const UPDATE = colors.yellow('[~]'); const REMOVAL = colors.red('[-]'); class Formatter { - constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }, diff?: TemplateDiff) { + constructor(private readonly stream: NodeJS.WriteStream, + private readonly logicalToPathMap: { [logicalId: string]: string }, + diff?: TemplateDiff, + private readonly context: number = 5) { // Read additional construct paths from the diff if it is supplied if (diff) { this.readConstructPathsFrom(diff); @@ -126,7 +141,7 @@ class Formatter { * Print a resource difference for a given logical ID. * * @param logicalId the logical ID of the resource that changed. - * @param diff the change to be rendered. + * @param diff the change to be rendered. */ public formatResourceDifference(_type: string, logicalId: string, diff: ResourceDifference) { const resourceType = diff.isRemoval ? diff.oldResourceType : diff.newResourceType; @@ -184,9 +199,9 @@ class Formatter { /** * Renders a tree of differences under a particular name. - * @param name the name of the root of the tree. - * @param diff the difference on the tree. - * @param last whether this is the last node of a parent tree. + * @param name the name of the root of the tree. + * @param diff the difference on the tree. + * @param last whether this is the last node of a parent tree. */ public formatTreeDiff(name: string, diff: Difference, last: boolean) { let additionalInfo = ''; @@ -210,10 +225,19 @@ class Formatter { * @param linePrefix a prefix (indent-like) to be used on every line. */ public formatObjectDiff(oldObject: any, newObject: any, linePrefix: string) { - if ((typeof oldObject !== typeof newObject) || Array.isArray(oldObject) || typeof oldObject === 'string' || typeof oldObject === 'number') { + if ((typeof oldObject !== typeof newObject) || Array.isArray(oldObject) || typeof oldObject === 'string' || typeof oldObject === 'number') { if (oldObject !== undefined && newObject !== undefined) { - this.print('%s ├─ %s %s', linePrefix, REMOVAL, this.formatValue(oldObject, colors.red)); - this.print('%s └─ %s %s', linePrefix, ADDITION, this.formatValue(newObject, colors.green)); + if (typeof oldObject === 'object' || typeof newObject === 'object') { + const oldStr = JSON.stringify(oldObject, null, 2); + const newStr = JSON.stringify(newObject, null, 2); + const diff = _diffStrings(oldStr, newStr, this.context); + for (let i = 0 ; i < diff.length ; i++) { + this.print('%s %s %s', linePrefix, i === 0 ? '└─' : ' ', diff[i]); + } + } else { + this.print('%s ├─ %s %s', linePrefix, REMOVAL, this.formatValue(oldObject, colors.red)); + this.print('%s └─ %s %s', linePrefix, ADDITION, this.formatValue(newObject, colors.green)); + } } else if (oldObject !== undefined /* && newObject === undefined */) { this.print('%s └─ %s', linePrefix, this.formatValue(oldObject, colors.red)); } else /* if (oldObject === undefined && newObject !== undefined) */ { @@ -398,3 +422,75 @@ function stripHorizontalLines(tableRendering: string) { return cols[1]; } } + +/** + * A patch as returned by ``diff.structuredPatch``. + */ +interface Patch { + /** + * Hunks in the patch. + */ + hunks: ReadonlyArray; +} + +/** + * A hunk in a patch produced by ``diff.structuredPatch``. + */ +interface PatchHunk { + oldStart: number; + oldLines: number; + newStart: number; + newLines: number; + lines: string[]; +} + +/** + * Creates a unified diff of two strings. + * + * @param oldStr the "old" version of the string. + * @param newStr the "new" version of the string. + * @param context the number of context lines to use in arbitrary JSON diff. + * + * @returns an array of diff lines. + */ +function _diffStrings(oldStr: string, newStr: string, context: number): string[] { + const patch: Patch = structuredPatch(null, null, oldStr, newStr, null, null, { context }); + const result = new Array(); + for (const hunk of patch.hunks) { + result.push(colors.magenta(`@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`)); + const baseIndent = _findIndent(hunk.lines); + for (const line of hunk.lines) { + // Don't care about termination newline. + if (line === '\\ No newline at end of file') { continue; } + const marker = line.charAt(0); + const text = line.slice(1 + baseIndent); + switch (marker) { + case ' ': + result.push(`${CONTEXT} ${text}`); + break; + case '+': + result.push(colors.bold(`${ADDITION} ${colors.green(text)}`)); + break; + case '-': + result.push(colors.bold(`${REMOVAL} ${colors.red(text)}`)); + break; + default: + throw new Error(`Unexpected diff marker: ${marker} (full line: ${line})`); + } + } + } + return result; + + function _findIndent(lines: string[]): number { + let indent = Number.MAX_SAFE_INTEGER; + for (const line of lines) { + for (let i = 1 ; i < line.length ; i++) { + if (line.charAt(i) !== ' ') { + indent = indent > i - 1 ? i - 1 : indent; + break; + } + } + } + return indent; + } +} diff --git a/packages/@aws-cdk/cloudformation-diff/package-lock.json b/packages/@aws-cdk/cloudformation-diff/package-lock.json index 7ac316ae5efc2..b8036bf0336eb 100644 --- a/packages/@aws-cdk/cloudformation-diff/package-lock.json +++ b/packages/@aws-cdk/cloudformation-diff/package-lock.json @@ -1,6 +1,6 @@ { "name": "@aws-cdk/cloudformation-diff", - "version": "0.18.1", + "version": "0.21.0", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -35,6 +35,11 @@ "resolved": "https://registry.npmjs.org/colors/-/colors-1.3.2.tgz", "integrity": "sha512-rhP0JSBGYvpcNQj4s5AdShMeE5ahMop96cTeDl/v9qQQm2fYClE2QXZRi8wLzc+GmXSxdIqqbOIAhyObEXDbfQ==" }, + "diff": { + "version": "4.0.1", + "resolved": "https://registry.npmjs.org/diff/-/diff-4.0.1.tgz", + "integrity": "sha512-s2+XdvhPCOF01LRQBC8hf4vhbVmI2CGS5aZnxLJlT5FtdhPCDFq80q++zK2KlrVorVDdL5BOGZ/VfLrVtYNF+Q==" + }, "fast-check": { "version": "1.8.0", "resolved": "https://registry.npmjs.org/fast-check/-/fast-check-1.8.0.tgz", diff --git a/packages/@aws-cdk/cloudformation-diff/package.json b/packages/@aws-cdk/cloudformation-diff/package.json index 19f92a3197b1a..2d6473d7d4196 100644 --- a/packages/@aws-cdk/cloudformation-diff/package.json +++ b/packages/@aws-cdk/cloudformation-diff/package.json @@ -27,6 +27,7 @@ "@aws-cdk/cx-api": "^0.21.0", "cli-table": "^0.3.1", "colors": "^1.2.1", + "diff": "^4.0.1", "fast-deep-equal": "^2.0.1", "source-map-support": "^0.5.6" }, diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index bd3bd03c316f4..d4a4ab52dd6c5 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -60,6 +60,7 @@ async function parseCommandLineArguments() { .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) .command('diff [STACK]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs + .option('json-context', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 5 }) .option('template', { type: 'string', desc: 'the path to the CloudFormation template to compare with' }) .option('strict', { type: 'boolean', desc: 'do not filter out AWS::CDK::Metadata resources', default: false })) .command('metadata [STACK]', 'Returns all metadata associated with this stack') @@ -142,7 +143,7 @@ async function initCommandLine() { return returnValue; } - async function main(command: string, args: any): Promise { + async function main(command: string, args: any): Promise { const toolkitStackName: string = configuration.combined.get(['toolkitStackName']) || DEFAULT_TOOLKIT_STACK_NAME; if (toolkitStackName !== DEFAULT_TOOLKIT_STACK_NAME) { @@ -158,7 +159,7 @@ async function initCommandLine() { return await cliList({ long: args.long }); case 'diff': - return await diffStack(await findStack(args.STACK), args.template, args.strict); + return await diffStack(await findStack(args.STACK), args.template, args.strict, args.jsonContext); case 'bootstrap': return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn); @@ -383,10 +384,10 @@ async function initCommandLine() { } } - async function diffStack(stackName: string, templatePath: string | undefined, strict: boolean): Promise { + async function diffStack(stackName: string, templatePath: string | undefined, strict: boolean, context: number): Promise { const stack = await appStacks.synthesizeStack(stackName); const currentTemplate = await readCurrentTemplate(stack, templatePath); - if (printStackDiff(currentTemplate, stack, strict) === 0) { + if (printStackDiff(currentTemplate, stack, strict, context) === 0) { return 0; } else { return 1; diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index cc83d9fc88178..5a56a3fc6b1a0 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -8,10 +8,12 @@ import { print, warning } from './logging'; * * @param oldTemplate the old/current state of the stack. * @param newTemplate the new/target state of the stack. + * @param strict do not filter out AWS::CDK::Metadata + * @param context lines of context to use in arbitrary JSON diff * * @returns the count of differences that were rendered. */ -export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack, strict: boolean): number { +export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedStack, strict: boolean, context: number): number { if (_hasAssets(newTemplate)) { const issue = 'https://github.com/awslabs/aws-cdk/issues/395'; warning(`The ${newTemplate.name} stack uses assets, which are currently not accounted for in the diff output! See ${issue}`); @@ -30,7 +32,7 @@ export function printStackDiff(oldTemplate: any, newTemplate: cxapi.SynthesizedS } if (!diff.isEmpty) { - cfnDiff.formatDifferences(process.stderr, diff, buildLogicalToPathMap(newTemplate)); + cfnDiff.formatDifferences(process.stderr, diff, buildLogicalToPathMap(newTemplate), context); } else { print(colors.green('There were no differences')); } From af63712c63c8e0caf383780fa6d6516aab099e13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 7 Jan 2019 15:09:22 +0100 Subject: [PATCH 2/4] PR comments --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 0c18dbcdbf405..800cbab88bc02 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -14,7 +14,7 @@ const { structuredPatch } = require('diff'); /** * Renders template differences to the process' console. * - * @param string The IO stream where to output the rendered diff. + * @param stream The IO stream where to output the rendered diff. * @param templateDiff TemplateDiff to be rendered to the console. * @param logicalToPathMap A map from logical ID to construct path. Useful in * case there is no aws:cdk:path metadata in the template. @@ -23,7 +23,7 @@ const { structuredPatch } = require('diff'); export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }, - context?: number) { + context: number = 5) { const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context); if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) { From 0bd71d4c2a41d616e298b8c224bc4e3f3c673d23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 7 Jan 2019 15:17:03 +0100 Subject: [PATCH 3/4] Rename --json-context to --context-lines --- packages/aws-cdk/bin/cdk.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index d4a4ab52dd6c5..0a25a78159cf8 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -60,7 +60,7 @@ async function parseCommandLineArguments() { .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) .command('diff [STACK]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs - .option('json-context', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 5 }) + .option('context-lines', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 5 }) .option('template', { type: 'string', desc: 'the path to the CloudFormation template to compare with' }) .option('strict', { type: 'boolean', desc: 'do not filter out AWS::CDK::Metadata resources', default: false })) .command('metadata [STACK]', 'Returns all metadata associated with this stack') @@ -159,7 +159,7 @@ async function initCommandLine() { return await cliList({ long: args.long }); case 'diff': - return await diffStack(await findStack(args.STACK), args.template, args.strict, args.jsonContext); + return await diffStack(await findStack(args.STACK), args.template, args.strict, args.contextLines); case 'bootstrap': return await cliBootstrap(args.ENVIRONMENTS, toolkitStackName, args.roleArn); From 9dcbaacf0a7ebc918b167dff008a9ed59b7d14c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 7 Jan 2019 15:32:51 +0100 Subject: [PATCH 4/4] Reduce default context to 3 lines --- packages/@aws-cdk/cloudformation-diff/lib/format.ts | 6 +++--- packages/aws-cdk/bin/cdk.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/format.ts b/packages/@aws-cdk/cloudformation-diff/lib/format.ts index 800cbab88bc02..c808dbc822497 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/format.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/format.ts @@ -18,12 +18,12 @@ const { structuredPatch } = require('diff'); * @param templateDiff TemplateDiff to be rendered to the console. * @param logicalToPathMap A map from logical ID to construct path. Useful in * case there is no aws:cdk:path metadata in the template. - * @param context the number of context lines to use in arbitrary JSON diff. + * @param context the number of context lines to use in arbitrary JSON diff (defaults to 3). */ export function formatDifferences(stream: NodeJS.WriteStream, templateDiff: TemplateDiff, logicalToPathMap: { [logicalId: string]: string } = { }, - context: number = 5) { + context: number = 3) { const formatter = new Formatter(stream, logicalToPathMap, templateDiff, context); if (templateDiff.awsTemplateFormatVersion || templateDiff.transform || templateDiff.description) { @@ -75,7 +75,7 @@ class Formatter { constructor(private readonly stream: NodeJS.WriteStream, private readonly logicalToPathMap: { [logicalId: string]: string }, diff?: TemplateDiff, - private readonly context: number = 5) { + private readonly context: number = 3) { // Read additional construct paths from the diff if it is supplied if (diff) { this.readConstructPathsFrom(diff); diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts index 0a25a78159cf8..600a9d86ec1f0 100644 --- a/packages/aws-cdk/bin/cdk.ts +++ b/packages/aws-cdk/bin/cdk.ts @@ -60,7 +60,7 @@ async function parseCommandLineArguments() { .command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', yargs => yargs .option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) .command('diff [STACK]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', yargs => yargs - .option('context-lines', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 5 }) + .option('context-lines', { type: 'number', desc: 'number of context lines to include in arbitrary JSON diff rendering', default: 3 }) .option('template', { type: 'string', desc: 'the path to the CloudFormation template to compare with' }) .option('strict', { type: 'boolean', desc: 'do not filter out AWS::CDK::Metadata resources', default: false })) .command('metadata [STACK]', 'Returns all metadata associated with this stack')