Skip to content

Commit 37caaab

Browse files
rix0rrrrix0rrr
authored
fix(cli): deployment continues if ECR asset fails to build or publish (#26060)
Fixes #26048, fixes #25827. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 3b14213 commit 37caaab

File tree

2 files changed

+98
-22
lines changed

2 files changed

+98
-22
lines changed

‎packages/aws-cdk/lib/api/deployments.ts

+6
Original file line numberDiff line numberDiff line change
@@ -589,6 +589,9 @@ export class Deployments {
589589

590590
const publisher = this.cachedPublisher(assetManifest, stackEnv, options.stackName);
591591
await publisher.buildEntry(asset);
592+
if (publisher.hasFailures) {
593+
throw new Error(`Failed to build asset ${asset.id}`);
594+
}
592595
}
593596

594597
/**
@@ -601,6 +604,9 @@ export class Deployments {
601604
// No need to validate anymore, we already did that during build
602605
const publisher = this.cachedPublisher(assetManifest, stackEnv, options.stackName);
603606
await publisher.publishEntry(asset);
607+
if (publisher.hasFailures) {
608+
throw new Error(`Failed to publish asset ${asset.id}`);
609+
}
604610
}
605611

606612
/**

‎packages/aws-cdk/test/work-graph.test.ts

+92-22
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,29 @@ describe('WorkGraph', () => {
3030

3131
actionedAssets.push(x.id);
3232
},
33-
buildAsset: async({ id }: AssetBuildNode) => {
34-
actionedAssets.push(id);
33+
buildAsset: async(x: AssetBuildNode) => {
34+
const errorMessage = x.parentStack.displayName;
35+
const timeout = Number(x.parentStack.stackName) || 0;
36+
37+
await sleep(timeout);
38+
39+
if (errorMessage) {
40+
throw Error(errorMessage);
41+
}
42+
43+
actionedAssets.push(x.id);
3544
},
36-
publishAsset: async({ id }: AssetPublishNode) => {
37-
actionedAssets.push(id);
45+
publishAsset: async(x: AssetPublishNode) => {
46+
const errorMessage = x.parentStack.displayName;
47+
const timeout = Number(x.parentStack.stackName) || 0;
48+
49+
await sleep(timeout);
50+
51+
if (errorMessage) {
52+
throw Error(errorMessage);
53+
}
54+
55+
actionedAssets.push(x.id);
3856
},
3957
};
4058

@@ -252,11 +270,11 @@ describe('WorkGraph', () => {
252270
// Failure Concurrency
253271
test.each([
254272
// Concurrency 1
255-
{ scenario: 'A (error)', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expectedStacks: [] },
256-
{ scenario: 'A (error), B', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack' }]), expectedError: 'A', expectedStacks: [] },
257-
{ scenario: 'A, B (error)', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack' }, { id: 'B', type: 'stack', displayName: 'B' }]), expectedError: 'B', expectedStacks: ['A'] },
258-
{ scenario: 'A (error) -> B', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack', stackDependencies: ['A'] }]), expectedError: 'A', expectedStacks: [] },
259-
{ scenario: '[unsorted] A (error) -> B', concurrency: 1, toDeploy: createArtifacts([{ id: 'B', type: 'stack', stackDependencies: ['A'] }, { id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expectedStacks: [] },
273+
{ scenario: 'A (error)', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expected: [] },
274+
{ scenario: 'A (error), B', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack' }]), expectedError: 'A', expected: [] },
275+
{ scenario: 'A, B (error)', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack' }, { id: 'B', type: 'stack', displayName: 'B' }]), expectedError: 'B', expected: ['A'] },
276+
{ scenario: 'A (error) -> B', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack', stackDependencies: ['A'] }]), expectedError: 'A', expected: [] },
277+
{ scenario: '[unsorted] A (error) -> B', concurrency: 1, toDeploy: createArtifacts([{ id: 'B', type: 'stack', stackDependencies: ['A'] }, { id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expected: [] },
260278
{
261279
scenario: 'A (error) -> B, C -> D',
262280
concurrency: 1,
@@ -267,7 +285,7 @@ describe('WorkGraph', () => {
267285
{ id: 'D', type: 'stack', stackDependencies: ['C'] },
268286
]),
269287
expectedError: 'A',
270-
expectedStacks: [],
288+
expected: [],
271289
},
272290
{
273291
scenario: 'A -> B, C (error) -> D',
@@ -279,15 +297,36 @@ describe('WorkGraph', () => {
279297
{ id: 'D', type: 'stack', stackDependencies: ['C'] },
280298
]),
281299
expectedError: 'C',
282-
expectedStacks: ['A'],
300+
expected: ['A'],
301+
},
302+
// With assets
303+
{
304+
scenario: 'A -> b (asset build error)',
305+
concurrency: 1,
306+
toDeploy: createArtifacts([
307+
{ id: 'A', type: 'stack', assetDependencies: ['b'] },
308+
{ id: 'b', type: 'asset', displayName: 'build-b' },
309+
]),
310+
expectedError: 'build-b',
311+
expected: [],
312+
},
313+
{
314+
scenario: 'A -> b (asset publish error)',
315+
concurrency: 1,
316+
toDeploy: createArtifacts([
317+
{ id: 'A', type: 'stack', assetDependencies: ['b'] },
318+
{ id: 'b', type: 'asset', displayName: 'publish-b' },
319+
]),
320+
expectedError: 'publish-b',
321+
expected: ['b-build'],
283322
},
284323

285324
// Concurrency 2
286-
{ scenario: 'A (error)', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expectedStacks: [] },
287-
{ scenario: 'A (error), B', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack' }]), expectedError: 'A', expectedStacks: ['B'] },
288-
{ scenario: 'A, B (error)', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack' }, { id: 'B', type: 'stack', displayName: 'B' }]), expectedError: 'B', expectedStacks: ['A'] },
289-
{ scenario: 'A (error) -> B', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack', stackDependencies: ['A'] }]), expectedError: 'A', expectedStacks: [] },
290-
{ scenario: '[unsorted] A (error) -> B', concurrency: 2, toDeploy: createArtifacts([{ id: 'B', type: 'stack', stackDependencies: ['A'] }, { id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expectedStacks: [] },
325+
{ scenario: 'A (error)', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expected: [] },
326+
{ scenario: 'A (error), B', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack' }]), expectedError: 'A', expected: ['B'] },
327+
{ scenario: 'A, B (error)', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack' }, { id: 'B', type: 'stack', displayName: 'B' }]), expectedError: 'B', expected: ['A'] },
328+
{ scenario: 'A (error) -> B', concurrency: 2, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }, { id: 'B', type: 'stack', stackDependencies: ['A'] }]), expectedError: 'A', expected: [] },
329+
{ scenario: '[unsorted] A (error) -> B', concurrency: 2, toDeploy: createArtifacts([{ id: 'B', type: 'stack', stackDependencies: ['A'] }, { id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expected: [] },
291330
{
292331
scenario: 'A (error) -> B, C -> D',
293332
concurrency: 2,
@@ -298,7 +337,7 @@ describe('WorkGraph', () => {
298337
{ id: 'D', type: 'stack', stackDependencies: ['C'] },
299338
]),
300339
expectedError: 'A',
301-
expectedStacks: ['C'],
340+
expected: ['C'],
302341
},
303342
{
304343
scenario: 'A -> B, C (error) -> D',
@@ -310,15 +349,38 @@ describe('WorkGraph', () => {
310349
{ id: 'D', type: 'stack', stackDependencies: ['C'] },
311350
]),
312351
expectedError: 'C',
313-
expectedStacks: ['A', 'B'],
352+
expected: ['A', 'B'],
353+
},
354+
// With assets
355+
{
356+
scenario: 'A -> b (asset build error), C',
357+
concurrency: 2,
358+
toDeploy: createArtifacts([
359+
{ id: 'A', type: 'stack', assetDependencies: ['b'] },
360+
{ id: 'b', type: 'asset', displayName: 'build-b' },
361+
{ id: 'C', type: 'stack' },
362+
]),
363+
expectedError: 'build-b',
364+
expected: ['C'],
365+
},
366+
{
367+
scenario: 'A -> b (asset publish error), C',
368+
concurrency: 2,
369+
toDeploy: createArtifacts([
370+
{ id: 'A', type: 'stack', assetDependencies: ['b'] },
371+
{ id: 'b', type: 'asset', displayName: 'publish-b' },
372+
{ id: 'C', type: 'stack' },
373+
]),
374+
expectedError: 'publish-b',
375+
expected: ['b-build', 'C'],
314376
},
315-
])('Failure - Concurrency: $concurrency - $scenario', async ({ concurrency, expectedError, toDeploy, expectedStacks }) => {
377+
])('Failure - Concurrency: $concurrency - $scenario', async ({ concurrency, expectedError, toDeploy, expected }) => {
316378
const graph = new WorkGraph();
317379
addTestArtifactsToGraph(toDeploy, graph);
318380

319381
await expect(graph.doParallel(concurrency, callbacks)).rejects.toThrowError(expectedError);
320382

321-
expect(actionedAssets).toStrictEqual(expectedStacks);
383+
expect(actionedAssets).toStrictEqual(expected);
322384
});
323385

324386
// Failure Graph Circular Dependencies
@@ -393,7 +455,11 @@ function addTestArtifactsToGraph(toDeploy: TestArtifact[], graph: WorkGraph) {
393455
asset: DUMMY,
394456
assetManifest: DUMMY,
395457
assetManifestArtifact: DUMMY,
396-
parentStack: DUMMY,
458+
parentStack: {
459+
// We're smuggling information here so that the set of callbacks can do some appropriate action
460+
stackName: node.name, // Used to smuggle sleep duration
461+
displayName: node.displayName?.includes('build') ? node.displayName : undefined, // Used to smuggle exception triggers
462+
} as any,
397463
dependencies: new Set([...node.stackDependencies ?? [], ...(node.assetDependencies ?? []).map(x => `${x}-publish`)]),
398464
});
399465
graph.addNodes({
@@ -403,7 +469,11 @@ function addTestArtifactsToGraph(toDeploy: TestArtifact[], graph: WorkGraph) {
403469
asset: DUMMY,
404470
assetManifest: DUMMY,
405471
assetManifestArtifact: DUMMY,
406-
parentStack: DUMMY,
472+
parentStack: {
473+
// We're smuggling information here so that the set of callbacks can do some appropriate action
474+
stackName: node.name, // Used to smuggle sleep duration
475+
displayName: node.displayName?.includes('publish') ? node.displayName : undefined, // Used to smuggle exception triggers
476+
} as any,
407477
dependencies: new Set([`${node.id}-build`]),
408478
});
409479
break;

0 commit comments

Comments
 (0)