diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 64dcb0d91e60a..4da85242d4c20 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -19,7 +19,6 @@ export class WorkGraphBuilder { 'stack': 5, }; private readonly graph = new WorkGraph(); - private readonly assetBuildNodes = new Map; constructor(private readonly prebuildAssets: boolean, private readonly idPrefix = '') { } @@ -39,12 +38,16 @@ export class WorkGraphBuilder { */ // eslint-disable-next-line max-len private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { - const buildId = `${this.idPrefix}${asset.id}-build`; + // Just the artifact identifier + const assetId = asset.id.assetId; + // Unique per destination where the artifact needs to go + const assetDestinationId = `${asset.id}`; - // Add the build node, but only one per "source" - // The genericSource includes a relative path we could make absolute to do more effective deduplication of build steps. Not doing that right now. - const assetBuildNodeKey = JSON.stringify(asset.genericSource); - if (!this.assetBuildNodes.has(assetBuildNodeKey)) { + const buildId = `${this.idPrefix}${assetId}-build`; + const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`; + + // 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, @@ -60,13 +63,9 @@ export class WorkGraphBuilder { deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-build'], }; - this.assetBuildNodes.set(assetBuildNodeKey, node); this.graph.addNodes(node); } - // Always add the publish - const publishNodeId = `${this.idPrefix}${asset.id}-publish`; - const publishNode = this.graph.tryGetNode(publishNodeId); if (!publishNode) { this.graph.addNodes({ diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index e89d8e35e911d..686affae6bfb6 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -3,6 +3,7 @@ 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'; +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'; @@ -36,14 +37,14 @@ describe('with some stacks and assets', () => { expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({ type: 'asset-publish', - dependencies: new Set(['F1:D1-build']), + dependencies: new Set(['F1-build']), } as 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:D1-build')).toEqual(expect.objectContaining({ + expect(graph.node('F1-build')).toEqual(expect.objectContaining({ type: 'asset-build', dependencies: new Set(['stack0', 'stack1']), } as Partial)); @@ -52,7 +53,7 @@ describe('with some stacks and assets', () => { test('with prebuild on, assets only have their own dependencies', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({ + expect(graph.node('F1-build')).toEqual(expect.objectContaining({ type: 'asset-build', dependencies: new Set(['stack0']), } as Partial)); @@ -138,17 +139,11 @@ describe('tests that use assets', () => { const assembly = rootBuilder.buildAssembly(); - const traversal: string[] = []; const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - await graph.doParallel(1, { - deployStack: async (node) => { traversal.push(node.id); }, - buildAsset: async (node) => { traversal.push(node.id); }, - publishAsset: async (node) => { traversal.push(node.id); }, - }); + const traversal = await traverseAndRecord(graph); - expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks expect(traversal).toEqual([ - 'work-graph-builder.test.js:D1-build', + 'work-graph-builder.test.js-build', 'work-graph-builder.test.js:D1-publish', 'StackA', 'StackB', @@ -171,6 +166,53 @@ describe('tests that use assets', () => { // THEN expect(graph.findCycle()).toBeUndefined(); }); + + test('the same asset to different destinations is only built once', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D1: { bucketName: 'bucket1', objectKey: 'key' }, + D2: { bucketName: 'bucket2', 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: { + D3: { bucketName: 'bucket3', objectKey: 'key' }, + }, + }, + }, + }); + + const assembly = rootBuilder.buildAssembly(); + + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + const traversal = await traverseAndRecord(graph); + + expect(traversal).toEqual([ + 'abcdef-build', + 'abcdef:D1-publish', + 'abcdef:D2-publish', + 'StackA', + 'abcdef:D3-publish', + 'StackB', + ]); + }); }); /** @@ -251,3 +293,13 @@ function assertableNode(x: A) { dependencies: Array.from(x.dependencies), }; } + +async function traverseAndRecord(graph: WorkGraph) { + const ret: string[] = []; + await graph.doParallel(1, { + deployStack: async (node) => { ret.push(node.id); }, + buildAsset: async (node) => { ret.push(node.id); }, + publishAsset: async (node) => { ret.push(node.id); }, + }); + return ret; +} \ No newline at end of file diff --git a/packages/cdk-assets/lib/asset-manifest.ts b/packages/cdk-assets/lib/asset-manifest.ts index 961200e024a18..857bbbc8b0144 100644 --- a/packages/cdk-assets/lib/asset-manifest.ts +++ b/packages/cdk-assets/lib/asset-manifest.ts @@ -106,7 +106,10 @@ export class AssetManifest { } /** - * List of assets, splat out to destinations + * List of assets per destination + * + * Returns one asset for every publishable destination. Multiple asset + * destinations may share the same asset source. */ public get entries(): IManifestEntry[] { return [ @@ -145,7 +148,7 @@ const ASSET_TYPES: AssetType[] = ['files', 'dockerImages']; */ export interface IManifestEntry { /** - * The identifier of the asset + * The identifier of the asset and its destination */ readonly id: DestinationIdentifier; @@ -209,10 +212,16 @@ export class DockerImageManifestEntry implements IManifestEntry { /** * Identify an asset destination in an asset manifest + * + * When stringified, this will be a combination of the source + * and destination IDs. */ export class DestinationIdentifier { /** * Identifies the asset, by source. + * + * The assetId will be the same between assets that represent + * the same physical file or image. */ public readonly assetId: string;