From aa8d9ca07aafe6b5f1cd97be714580e61f5cb1b6 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 14:58:26 +0200 Subject: [PATCH 1/7] fix(cli): asset not uploaded with different synthesizer configs If the same asset is used in 2 stacks that use different synthesizer configurations for publishing (for example, by using a different prefix) the asset will only be uploaded once instead of twice. We used to make the assumption that it was okay to use the destination ID as token of uniqueness. This is true inside a single manifest, but does not hold when there is more than stack that each have a manifest: both may have the destination ID `current_account:current_region`, but have different parameters for each destination. Instead, we calculate a content hash over the destination definition itself. That way, if the definitions are different we will create different nodes for each of them. Fixes #25927. --- packages/aws-cdk/lib/util/content-hash.ts | 39 +++++++ .../aws-cdk/lib/util/work-graph-builder.ts | 87 ++++++++------ packages/aws-cdk/lib/util/work-graph-types.ts | 2 + packages/aws-cdk/lib/util/work-graph.ts | 19 +++- .../aws-cdk/test/work-graph-builder.test.ts | 107 +++++++++++++++--- 5 files changed, 197 insertions(+), 57 deletions(-) diff --git a/packages/aws-cdk/lib/util/content-hash.ts b/packages/aws-cdk/lib/util/content-hash.ts index aa78d55dd688d..839295eee07ce 100644 --- a/packages/aws-cdk/lib/util/content-hash.ts +++ b/packages/aws-cdk/lib/util/content-hash.ts @@ -2,4 +2,43 @@ import * as crypto from 'crypto'; export function contentHash(data: string | Buffer | DataView) { return crypto.createHash('sha256').update(data).digest('hex'); +} + +/** + * A stably sorted Merkle hash of an arbitrary JS object + */ +export function contentHashAny(value: unknown) { + const ret = crypto.createHash('sha256'); + recurse(value); + return ret.digest('hex'); + + function recurse(x: unknown) { + if (typeof x === 'string') { + ret.update(x); + return; + } + + if (Array.isArray(x)) { + ret.update('['); + for (const e of x) { + recurse(e); + ret.update('||'); + } + ret.update(']'); + return; + } + + if (x && typeof x === 'object') { + ret.update('{'); + for (const key of Object.keys(x).sort()) { + ret.update(key); + ret.update(':'); + recurse((x as any)[key]); + } + ret.update('}'); + return; + } + + ret.update(`${x}`); + } } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 4da85242d4c20..277bda382b338 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -1,5 +1,6 @@ import * as cxapi from '@aws-cdk/cx-api'; import { AssetManifest, IManifestEntry } from 'cdk-assets'; +import { contentHashAny } from './content-hash'; import { WorkGraph } from './work-graph'; import { DeploymentState, AssetBuildNode, WorkNode } from './work-graph-types'; @@ -26,7 +27,7 @@ export class WorkGraphBuilder { this.graph.addNodes({ type: 'stack', id: `${this.idPrefix}${artifact.id}`, - dependencies: new Set(this.getDepIds(onlyStacks(artifact.dependencies))), + dependencies: new Set(this.stackArtifactIds(onlyStacks(artifact.dependencies))), stack: artifact, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES.stack, @@ -37,53 +38,53 @@ export class WorkGraphBuilder { * Oof, see this parameter list */ // eslint-disable-next-line max-len - private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { + private addAsset(ref: AssetReference) { // Just the artifact identifier - const assetId = asset.id.assetId; - // Unique per destination where the artifact needs to go - const assetDestinationId = `${asset.id}`; + const assetId = ref.manifestEntry.id.assetId; - const buildId = `${this.idPrefix}${assetId}-build`; - const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`; + const buildId = `build-${assetId}-${contentHashAny([assetId, ref.manifestEntry.genericSource]).substring(0, 10)}`; + const publishId = `publish-${assetId}-${contentHashAny([assetId, ref.manifestEntry.genericDestination]).substring(0, 10)}`; // Build node only gets added once because they are all the same if (!this.graph.tryGetNode(buildId)) { const node: AssetBuildNode = { type: 'asset-build', id: buildId, + note: assetId, dependencies: new Set([ - ...this.getDepIds(assetArtifact.dependencies), + ...this.stackArtifactIds(ref.manifestArtifact.dependencies), // If we disable prebuild, then assets inherit (stack) dependencies from their parent stack - ...!this.prebuildAssets ? this.getDepIds(onlyStacks(parentStack.dependencies)) : [], + ...!this.prebuildAssets ? this.stackArtifactIds(onlyStacks(ref.parentStack.dependencies)) : [], ]), - parentStack, - assetManifestArtifact: assetArtifact, - assetManifest, - asset, + parentStack: ref.parentStack, + assetManifestArtifact: ref.manifestArtifact, + assetManifest: ref.assetManifest, + asset: ref.manifestEntry, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-build'], }; this.graph.addNodes(node); } - const publishNode = this.graph.tryGetNode(publishNodeId); + const publishNode = this.graph.tryGetNode(publishId); if (!publishNode) { this.graph.addNodes({ type: 'asset-publish', - id: publishNodeId, + id: publishId, + note: `${ref.manifestEntry.id}`, dependencies: new Set([ buildId, ]), - parentStack, - assetManifestArtifact: assetArtifact, - assetManifest, - asset, + parentStack: ref.parentStack, + assetManifestArtifact: ref.manifestArtifact, + assetManifest: ref.assetManifest, + asset: ref.manifestEntry, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-publish'], }); } - for (const inheritedDep of this.getDepIds(onlyStacks(parentStack.dependencies))) { + for (const inheritedDep of this.stackArtifactIds(onlyStacks(ref.parentStack.dependencies))) { // The asset publish step also depends on the stacks that the parent depends on. // This is purely cosmetic: if we don't do this, the progress printing of asset publishing // is going to interfere with the progress bar of the stack deployment. We could remove this @@ -91,11 +92,11 @@ export class WorkGraphBuilder { // Note: this may introduce a cycle if one of the parent's dependencies is another stack that // depends on this asset. To workaround this we remove these cycles once all nodes have // been added to the graph. - this.graph.addDependency(publishNodeId, inheritedDep); + this.graph.addDependency(publishId, inheritedDep); } // This will work whether the stack node has been added yet or not - this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishNodeId); + this.graph.addDependency(`${this.idPrefix}${ref.parentStack.id}`, publishId); } public build(artifacts: cxapi.CloudArtifact[]): WorkGraph { @@ -105,14 +106,14 @@ export class WorkGraphBuilder { if (cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) { this.addStack(artifact); } else if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) { - const manifest = AssetManifest.fromFile(artifact.file); + const assetManifest = AssetManifest.fromFile(artifact.file); - for (const entry of manifest.entries) { + for (const asset of assetManifest.entries) { const parentStack = parentStacks.get(artifact); if (parentStack === undefined) { throw new Error('Found an asset manifest that is not associated with a stack'); } - this.addAsset(parentStack, artifact, manifest, entry); + this.addAsset({ parentStack, manifestArtifact: artifact, assetManifest, manifestEntry: asset }); } } else if (cxapi.NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact(artifact)) { const assembly = new cxapi.CloudAssembly(artifact.fullPath, { topoSort: false }); @@ -131,17 +132,15 @@ export class WorkGraphBuilder { return this.graph; } - private getDepIds(deps: cxapi.CloudArtifact[]): string[] { - const ids = []; - for (const artifact of deps) { - if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) { - // Depend on only the publish step. The publish step will depend on the build step on its own. - ids.push(`${this.idPrefix}${artifact.id}-publish`); - } else { - ids.push(`${this.idPrefix}${artifact.id}`); - } + private stackArtifactIds(deps: cxapi.CloudArtifact[]): string[] { + return deps.flatMap((d) => cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(d) ? [this.stackArtifactId(d)] : []); + } + + private stackArtifactId(artifact: cxapi.CloudArtifact): string { + if (!cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) { + throw new Error(`Can only call this on CloudFormationStackArtifact, got: ${artifact.constructor.name}`); } - return ids; + return `${this.idPrefix}${artifact.id}`; } /** @@ -174,4 +173,22 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) { function onlyStacks(artifacts: cxapi.CloudArtifact[]) { return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact); +} + +/** + * A reference to a single asset entry for a single stack + * + * Also holds references to the asset context: the stack, + * the artifact, the loaded manifest. + * + * The asset manifest has an unnecessarily bad structure to work with, but such is as it is. + */ +interface AssetReference { + /** + * The asset itself + */ + readonly manifestEntry: IManifestEntry; + readonly parentStack: cxapi.CloudFormationStackArtifact; + readonly manifestArtifact: cxapi.AssetManifestArtifact; + readonly assetManifest: AssetManifest; } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph-types.ts b/packages/aws-cdk/lib/util/work-graph-types.ts index 9da5c59d85ec0..4a20295bf5cc1 100644 --- a/packages/aws-cdk/lib/util/work-graph-types.ts +++ b/packages/aws-cdk/lib/util/work-graph-types.ts @@ -16,6 +16,8 @@ export interface WorkNodeCommon { readonly id: string; readonly dependencies: Set; deploymentState: DeploymentState; + /** Some readable information to attach to the id, which may be unreadable */ + readonly note?: string; } export interface StackNode extends WorkNodeCommon { diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index a06a969d5f0dd..e0e3336f34d25 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -217,19 +217,16 @@ export class WorkGraph { function renderNode(id: string, node: WorkNode): string[] { const ret = []; if (node.deploymentState === DeploymentState.COMPLETED) { - ret.push(` "${simplifyId(id)}" [style=filled,fillcolor=yellow];`); + ret.push(` ${gv(id, { style: 'filled', fillcolor: 'yellow', comment: node.note })};`); } else { - ret.push(` "${simplifyId(id)}";`); + ret.push(` ${gv(id, { comment: node.note })};`); } for (const dep of node.dependencies) { - ret.push(` "${simplifyId(id)}" -> "${simplifyId(dep)}";`); + ret.push(` ${gv(id)} -> ${gv(dep)};`); } return ret; } - function simplifyId(id: string) { - return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1'); - } } /** @@ -392,3 +389,13 @@ function sum(xs: number[]) { function retainOnly(xs: A[], pred: (x: A) => boolean) { xs.splice(0, xs.length, ...xs.filter(pred)); } + +function gv(id: string, attrs?: Record) { + const attrString = Object.entries(attrs ?? {}).flatMap(([k, v]) => v !== undefined ? [`${k}="${v}"`] : []).join(','); + + return attrString ? `"${simplifyId(id)}" [${attrString}]` : `"${simplifyId(id)}"`; +} + +function simplifyId(id: string) { + return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1'); +} \ No newline at end of file diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 686affae6bfb6..c317827025081 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -3,6 +3,8 @@ import * as path from 'path'; import * as cxschema from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import { CloudAssemblyBuilder } from '@aws-cdk/cx-api'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { expect } from '@jest/globals'; import { WorkGraph } from '../lib/util/work-graph'; import { WorkGraphBuilder } from '../lib/util/work-graph-builder'; import { AssetBuildNode, AssetPublishNode, StackNode, WorkNode } from '../lib/util/work-graph-types'; @@ -16,6 +18,25 @@ afterEach(() => { rootBuilder.delete(); }); +function superset(xs: A[]): Set { + const ret = new Set(xs); + (ret as any).isSuperset = true; + return ret; +} + +expect.addEqualityTesters([ + function(exp: unknown, act: unknown): boolean | undefined { + if (exp instanceof Set && isIterable(act)) { + if ((exp as any).isSuperset) { + const actSet = new Set(act); + return Array.from(exp as any).every((x) => actSet.has(x)); + } + return this.equals(Array.from(exp).sort(), Array.from(act).sort()); + } + return undefined; + }, +]); + describe('with some stacks and assets', () => { let assembly: cxapi.CloudAssembly; beforeEach(() => { @@ -28,34 +49,36 @@ describe('with some stacks and assets', () => { expect(assertableNode(graph.node('stack2'))).toEqual(expect.objectContaining({ type: 'stack', - dependencies: expect.arrayContaining(['F1:D1-publish']), - } as StackNode)); + dependencies: superset(['publish-F1-add54bdbcb']), + } as Partial)); }); test('asset publishing step depends on asset building step', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + // eslint-disable-next-line no-console + console.log(graph.toString()); - expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({ + expect(graph.node('publish-F1-add54bdbcb')).toEqual(expect.objectContaining({ type: 'asset-publish', - dependencies: new Set(['F1-build']), - } as Partial)); + dependencies: superset(['build-F1-a533139934']), + } satisfies Partial)); }); test('with prebuild off, asset building inherits dependencies from their parent stack', () => { const graph = new WorkGraphBuilder(false).build(assembly.artifacts); - expect(graph.node('F1-build')).toEqual(expect.objectContaining({ + expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({ type: 'asset-build', - dependencies: new Set(['stack0', 'stack1']), + dependencies: superset(['stack0', 'stack1']), } as Partial)); }); test('with prebuild on, assets only have their own dependencies', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - expect(graph.node('F1-build')).toEqual(expect.objectContaining({ + expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({ type: 'asset-build', - dependencies: new Set(['stack0']), + dependencies: superset(['stack0']), } as Partial)); }); }); @@ -84,13 +107,16 @@ test('can handle nested assemblies', async () => { let workDone = 0; const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + await graph.doParallel(10, { deployStack: async () => { workDone += 1; }, buildAsset: async () => { }, publishAsset: async () => { workDone += 1; }, }); - expect(workDone).toEqual(8); + // The asset is shared between parent assembly and nested assembly, but the stacks will be deployed + // 3 stacks + 1 asset + 3 stacks (1 reused asset) + expect(workDone).toEqual(7); }); test('dependencies on unselected artifacts are silently ignored', async () => { @@ -143,8 +169,8 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'work-graph-builder.test.js-build', - 'work-graph-builder.test.js:D1-publish', + 'build-work-graph-builder.test.js-0aa42b8a73', + 'publish-work-graph-builder.test.js-5afe4c5879', 'StackA', 'StackB', ]); @@ -205,11 +231,56 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'abcdef-build', - 'abcdef:D1-publish', - 'abcdef:D2-publish', + 'build-abcdef-98b29b323e', + 'publish-abcdef-250b23e9ad', + 'publish-abcdef-422bc84ff9', + 'StackA', + 'publish-abcdef-e6c9d7e973', + 'StackB', + ]); + }); + + test('different parameters for the same named definition are both published', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D: { bucketName: 'bucket1', objectKey: 'key' }, + }, + }, + }, + }); + + addStack(rootBuilder, 'StackB', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackB.assets', 'StackA'], + }); + addAssets(rootBuilder, 'StackB.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D: { bucketName: 'bucket2', objectKey: 'key' }, + }, + }, + }, + }); + + const assembly = rootBuilder.buildAssembly(); + + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + const traversal = await traverseAndRecord(graph); + + expect(traversal).toEqual([ + 'build-abcdef-98b29b323e', + 'publish-abcdef-250b23e9ad', 'StackA', - 'abcdef:D3-publish', + 'publish-abcdef-422bc84ff9', 'StackB', ]); }); @@ -302,4 +373,8 @@ async function traverseAndRecord(graph: WorkGraph) { publishAsset: async (node) => { ret.push(node.id); }, }); return ret; +} + +function isIterable(x: unknown): x is Iterable { + return x && typeof x === 'object' && (x as any)[Symbol.iterator]; } \ No newline at end of file From e811dda165b232a26b71b15eb32b6b4a859c7b11 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 15:09:55 +0200 Subject: [PATCH 2/7] Undo addition of AssetReference --- .../aws-cdk/lib/util/work-graph-builder.ts | 56 +++++++------------ 1 file changed, 19 insertions(+), 37 deletions(-) diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 277bda382b338..54834047b36ca 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -38,12 +38,12 @@ export class WorkGraphBuilder { * Oof, see this parameter list */ // eslint-disable-next-line max-len - private addAsset(ref: AssetReference) { + private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetManifestArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { // Just the artifact identifier - const assetId = ref.manifestEntry.id.assetId; + const assetId = asset.id.assetId; - const buildId = `build-${assetId}-${contentHashAny([assetId, ref.manifestEntry.genericSource]).substring(0, 10)}`; - const publishId = `publish-${assetId}-${contentHashAny([assetId, ref.manifestEntry.genericDestination]).substring(0, 10)}`; + const buildId = `build-${assetId}-${contentHashAny([assetId, asset.genericSource]).substring(0, 10)}`; + const publishId = `publish-${assetId}-${contentHashAny([assetId, asset.genericDestination]).substring(0, 10)}`; // Build node only gets added once because they are all the same if (!this.graph.tryGetNode(buildId)) { @@ -52,14 +52,14 @@ export class WorkGraphBuilder { id: buildId, note: assetId, dependencies: new Set([ - ...this.stackArtifactIds(ref.manifestArtifact.dependencies), + ...this.stackArtifactIds(assetManifestArtifact.dependencies), // If we disable prebuild, then assets inherit (stack) dependencies from their parent stack - ...!this.prebuildAssets ? this.stackArtifactIds(onlyStacks(ref.parentStack.dependencies)) : [], + ...!this.prebuildAssets ? this.stackArtifactIds(onlyStacks(parentStack.dependencies)) : [], ]), - parentStack: ref.parentStack, - assetManifestArtifact: ref.manifestArtifact, - assetManifest: ref.assetManifest, - asset: ref.manifestEntry, + parentStack: parentStack, + assetManifestArtifact, + assetManifest, + asset, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-build'], }; @@ -71,20 +71,20 @@ export class WorkGraphBuilder { this.graph.addNodes({ type: 'asset-publish', id: publishId, - note: `${ref.manifestEntry.id}`, + note: `${asset.id}`, dependencies: new Set([ buildId, ]), - parentStack: ref.parentStack, - assetManifestArtifact: ref.manifestArtifact, - assetManifest: ref.assetManifest, - asset: ref.manifestEntry, + parentStack, + assetManifestArtifact, + assetManifest, + asset, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-publish'], }); } - for (const inheritedDep of this.stackArtifactIds(onlyStacks(ref.parentStack.dependencies))) { + for (const inheritedDep of this.stackArtifactIds(onlyStacks(parentStack.dependencies))) { // The asset publish step also depends on the stacks that the parent depends on. // This is purely cosmetic: if we don't do this, the progress printing of asset publishing // is going to interfere with the progress bar of the stack deployment. We could remove this @@ -96,7 +96,7 @@ export class WorkGraphBuilder { } // This will work whether the stack node has been added yet or not - this.graph.addDependency(`${this.idPrefix}${ref.parentStack.id}`, publishId); + this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishId); } public build(artifacts: cxapi.CloudArtifact[]): WorkGraph { @@ -108,12 +108,12 @@ export class WorkGraphBuilder { } else if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) { const assetManifest = AssetManifest.fromFile(artifact.file); - for (const asset of assetManifest.entries) { + for (const entry of assetManifest.entries) { const parentStack = parentStacks.get(artifact); if (parentStack === undefined) { throw new Error('Found an asset manifest that is not associated with a stack'); } - this.addAsset({ parentStack, manifestArtifact: artifact, assetManifest, manifestEntry: asset }); + this.addAsset(parentStack, artifact, assetManifest, entry); } } else if (cxapi.NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact(artifact)) { const assembly = new cxapi.CloudAssembly(artifact.fullPath, { topoSort: false }); @@ -174,21 +174,3 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) { function onlyStacks(artifacts: cxapi.CloudArtifact[]) { return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact); } - -/** - * A reference to a single asset entry for a single stack - * - * Also holds references to the asset context: the stack, - * the artifact, the loaded manifest. - * - * The asset manifest has an unnecessarily bad structure to work with, but such is as it is. - */ -interface AssetReference { - /** - * The asset itself - */ - readonly manifestEntry: IManifestEntry; - readonly parentStack: cxapi.CloudFormationStackArtifact; - readonly manifestArtifact: cxapi.AssetManifestArtifact; - readonly assetManifest: AssetManifest; -} \ No newline at end of file From 704393e205627e40b893991460533b7d9abb331e Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 15:10:21 +0200 Subject: [PATCH 3/7] Undo rename --- packages/aws-cdk/lib/util/work-graph-builder.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 54834047b36ca..1c61d307014a8 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -106,14 +106,14 @@ export class WorkGraphBuilder { if (cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) { this.addStack(artifact); } else if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) { - const assetManifest = AssetManifest.fromFile(artifact.file); + const manifest = AssetManifest.fromFile(artifact.file); - for (const entry of assetManifest.entries) { + for (const entry of manifest.entries) { const parentStack = parentStacks.get(artifact); if (parentStack === undefined) { throw new Error('Found an asset manifest that is not associated with a stack'); } - this.addAsset(parentStack, artifact, assetManifest, entry); + this.addAsset(parentStack, artifact, manifest, entry); } } else if (cxapi.NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact(artifact)) { const assembly = new cxapi.CloudAssembly(artifact.fullPath, { topoSort: false }); From 0673fb9bd9a1816d3eb0a4ec13fd8ba42fecb014 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 15:17:13 +0200 Subject: [PATCH 4/7] It's not a Merkle hash --- packages/aws-cdk/lib/util/content-hash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/util/content-hash.ts b/packages/aws-cdk/lib/util/content-hash.ts index 839295eee07ce..8e50a80a87ec7 100644 --- a/packages/aws-cdk/lib/util/content-hash.ts +++ b/packages/aws-cdk/lib/util/content-hash.ts @@ -5,7 +5,7 @@ export function contentHash(data: string | Buffer | DataView) { } /** - * A stably sorted Merkle hash of an arbitrary JS object + * A stably sorted hash of an arbitrary JS object */ export function contentHashAny(value: unknown) { const ret = crypto.createHash('sha256'); From e00882cc26196413619b0531dabc2efba06b62cc Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 15:19:29 +0200 Subject: [PATCH 5/7] Fix problem in the hash --- packages/aws-cdk/lib/util/content-hash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/util/content-hash.ts b/packages/aws-cdk/lib/util/content-hash.ts index 8e50a80a87ec7..fefe2dfb2ee1c 100644 --- a/packages/aws-cdk/lib/util/content-hash.ts +++ b/packages/aws-cdk/lib/util/content-hash.ts @@ -39,6 +39,6 @@ export function contentHashAny(value: unknown) { return; } - ret.update(`${x}`); + ret.update(`${x}${typeof x}`); // typeof to make sure hash('123') !== hash(123) } } \ No newline at end of file From e854ad2181bd3fab54497f1329291dc3411f9039 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 28 Aug 2023 15:34:52 +0200 Subject: [PATCH 6/7] Hashes --- packages/aws-cdk/test/work-graph-builder.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index c317827025081..555428a59d1b8 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -55,8 +55,6 @@ describe('with some stacks and assets', () => { test('asset publishing step depends on asset building step', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - // eslint-disable-next-line no-console - console.log(graph.toString()); expect(graph.node('publish-F1-add54bdbcb')).toEqual(expect.objectContaining({ type: 'asset-publish', @@ -169,7 +167,7 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-work-graph-builder.test.js-0aa42b8a73', + 'build-work-graph-builder.test.js-0c45de20db', 'publish-work-graph-builder.test.js-5afe4c5879', 'StackA', 'StackB', @@ -231,7 +229,7 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-abcdef-98b29b323e', + 'build-abcdef-c414bd1cfd', 'publish-abcdef-250b23e9ad', 'publish-abcdef-422bc84ff9', 'StackA', @@ -277,7 +275,7 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-abcdef-98b29b323e', + 'build-abcdef-c414bd1cfd', 'publish-abcdef-250b23e9ad', 'StackA', 'publish-abcdef-422bc84ff9', From 2f5856f05805fade25992fb04b547e3da02ddee8 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 29 Aug 2023 10:03:13 +0200 Subject: [PATCH 7/7] Make matching more reliable --- .../aws-cdk/test/work-graph-builder.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 555428a59d1b8..a8457bc48f639 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -167,8 +167,8 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-work-graph-builder.test.js-0c45de20db', - 'publish-work-graph-builder.test.js-5afe4c5879', + expect.stringMatching(/^build-work-graph-builder.test.js-.*$/), + expect.stringMatching(/^publish-work-graph-builder.test.js-.*$/), 'StackA', 'StackB', ]); @@ -229,11 +229,11 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-abcdef-c414bd1cfd', - 'publish-abcdef-250b23e9ad', - 'publish-abcdef-422bc84ff9', + expect.stringMatching(/^build-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), 'StackA', - 'publish-abcdef-e6c9d7e973', + expect.stringMatching(/^publish-abcdef-.*$/), 'StackB', ]); }); @@ -275,10 +275,10 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'build-abcdef-c414bd1cfd', - 'publish-abcdef-250b23e9ad', + expect.stringMatching(/^build-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), 'StackA', - 'publish-abcdef-422bc84ff9', + expect.stringMatching(/^publish-abcdef-.*$/), 'StackB', ]); });