Skip to content

Commit 26dcc1e

Browse files
comcalvicomcalvi
authored
fix(CLI): diff reports wrong count of changed stacks (#26796)
The current logic counts the number of changes in any parent stacks. It doesn't count nested stacks correctly, and doesn't count parent stacks correctly. With this change: <img width="1047" alt="Screenshot 2023-08-18 at 9 43 49 AM" src="https://github.com/aws/aws-cdk/assets/66279577/b417baa7-58d9-454a-a735-4bd406f1c126"> Without this change: <img width="1047" alt="Screenshot 2023-08-18 at 9 51 55 AM" src="https://github.com/aws/aws-cdk/assets/66279577/85e87e72-25ec-47af-96af-f47f7c43a4f2"> Closes #26818. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 95f8cef commit 26dcc1e

File tree

5 files changed

+135
-38
lines changed

5 files changed

+135
-38
lines changed

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

+7-3
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { ISDK } from './aws-auth/sdk';
66
import { CredentialsOptions, SdkForEnvironment, SdkProvider } from './aws-auth/sdk-provider';
77
import { deployStack, DeployStackResult, destroyStack, makeBodyParameterAndUpload, DeploymentMethod } from './deploy-stack';
88
import { HotswapMode } from './hotswap/common';
9-
import { loadCurrentTemplateWithNestedStacks, loadCurrentTemplate } from './nested-stack-helpers';
9+
import { loadCurrentTemplateWithNestedStacks, loadCurrentTemplate, flattenNestedStackNames, TemplateWithNestedStackCount } from './nested-stack-helpers';
1010
import { ToolkitInfo } from './toolkit-info';
1111
import { CloudFormationStack, Template, ResourcesToImport, ResourceIdentifierSummaries } from './util/cloudformation';
1212
import { StackActivityProgress } from './util/cloudformation/stack-activity-monitor';
@@ -300,9 +300,13 @@ export class Deployments {
300300
public async readCurrentTemplateWithNestedStacks(
301301
rootStackArtifact: cxapi.CloudFormationStackArtifact,
302302
retrieveProcessedTemplate: boolean = false,
303-
): Promise<Template> {
303+
): Promise<TemplateWithNestedStackCount> {
304304
const sdk = (await this.prepareSdkWithLookupOrDeployRole(rootStackArtifact)).stackSdk;
305-
return (await loadCurrentTemplateWithNestedStacks(rootStackArtifact, sdk, retrieveProcessedTemplate)).deployedTemplate;
305+
const templateWithNestedStacks = await loadCurrentTemplateWithNestedStacks(rootStackArtifact, sdk, retrieveProcessedTemplate);
306+
return {
307+
deployedTemplate: templateWithNestedStacks.deployedTemplate,
308+
nestedStackCount: flattenNestedStackNames(templateWithNestedStacks.nestedStackNames).length,
309+
};
306310
}
307311

308312
public async readCurrentTemplate(stackArtifact: cxapi.CloudFormationStackArtifact): Promise<Template> {

‎packages/aws-cdk/lib/api/nested-stack-helpers.ts

+28
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ export interface NestedStackNames {
1515
readonly nestedChildStackNames: { [logicalId: string]: NestedStackNames };
1616
}
1717

18+
export interface TemplateWithNestedStackCount {
19+
readonly deployedTemplate: Template;
20+
readonly nestedStackCount: number;
21+
}
22+
1823
/**
1924
* Reads the currently deployed template from CloudFormation and adds a
2025
* property, `NestedTemplate`, to any nested stacks that appear in either
@@ -40,6 +45,19 @@ export async function loadCurrentTemplateWithNestedStacks(
4045
};
4146
}
4247

48+
export function flattenNestedStackNames(nestedStackNames: { [nestedStackLogicalId: string]: NestedStackNames }): string[] {
49+
const nameList = [];
50+
for (const key of Object.keys(nestedStackNames)) {
51+
nameList.push(key);
52+
53+
if (Object.keys(nestedStackNames[key].nestedChildStackNames).length !== 0) {
54+
flattenNestedStacksHelper(nestedStackNames[key].nestedChildStackNames, nameList);
55+
}
56+
}
57+
58+
return nameList;
59+
}
60+
4361
/**
4462
* Returns the currently deployed template from CloudFormation that corresponds to `stackArtifact`.
4563
*/
@@ -135,6 +153,16 @@ function isCdkManagedNestedStack(stackResource: any): stackResource is NestedSta
135153
return stackResource.Type === 'AWS::CloudFormation::Stack' && stackResource.Metadata && stackResource.Metadata['aws:asset:path'];
136154
}
137155

156+
function flattenNestedStacksHelper(nestedStackNames: { [logicalId: string]: NestedStackNames }, nameList: string[]) {
157+
for (const key of Object.keys(nestedStackNames)) {
158+
nameList.push(key);
159+
160+
if (Object.keys(nestedStackNames[key].nestedChildStackNames).length !== 0) {
161+
flattenNestedStacksHelper(nestedStackNames[key].nestedChildStackNames, nameList);
162+
}
163+
}
164+
}
165+
138166
interface StackTemplates {
139167
readonly generatedTemplate: any;
140168
readonly deployedTemplate: any;

‎packages/aws-cdk/lib/cdk-toolkit.ts

+12-4
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,18 @@ export class CdkToolkit {
140140
stream.write(format('Stack %s\n', chalk.bold(stack.displayName)));
141141
}
142142

143-
const currentTemplate = await this.props.deployments.readCurrentTemplateWithNestedStacks(stack, options.compareAgainstProcessedTemplate);
144-
diffs += options.securityOnly
145-
? numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening))
146-
: printStackDiff(currentTemplate, stack, strict, contextLines, quiet, stream);
143+
const templateWithNames = await this.props.deployments.readCurrentTemplateWithNestedStacks(
144+
stack, options.compareAgainstProcessedTemplate,
145+
);
146+
const currentTemplate = templateWithNames.deployedTemplate;
147+
const nestedStackCount = templateWithNames.nestedStackCount;
148+
149+
const stackCount =
150+
options.securityOnly
151+
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening)) > 0 ? 1 : 0)
152+
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, stream) > 0 ? 1 : 0);
153+
154+
diffs += stackCount + nestedStackCount;
147155
}
148156
}
149157

‎packages/aws-cdk/test/api/cloudformation-deployments.test.ts

+12-4
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,11 @@ test('readCurrentTemplateWithNestedStacks() can handle non-Resources in the temp
246246
);
247247

248248
// WHEN
249-
const deployedTemplate = await deployments.readCurrentTemplateWithNestedStacks(rootStack);
249+
const nestedStackCount = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).nestedStackCount;
250+
const deployedTemplate = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).deployedTemplate;
250251

251252
// THEN
253+
expect(nestedStackCount).toEqual(1);
252254
expect(deployedTemplate).toEqual({
253255
Resources: {
254256
NestedStack: {
@@ -451,9 +453,11 @@ test('readCurrentTemplateWithNestedStacks() with a 3-level nested + sibling stru
451453
);
452454

453455
// WHEN
454-
const deployedTemplate = await deployments.readCurrentTemplateWithNestedStacks(rootStack);
456+
const nestedStackCount = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).nestedStackCount;
457+
const deployedTemplate = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).deployedTemplate;
455458

456459
// THEN
460+
expect(nestedStackCount).toEqual(3);
457461
expect(deployedTemplate).toEqual({
458462
Resources: {
459463
NestedStack: {
@@ -608,9 +612,11 @@ test('readCurrentTemplateWithNestedStacks() on an undeployed parent stack with a
608612
});
609613

610614
// WHEN
611-
const deployedTemplate = await deployments.readCurrentTemplateWithNestedStacks(rootStack);
615+
const nestedStackCount = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).nestedStackCount;
616+
const deployedTemplate = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).deployedTemplate;
612617

613618
// THEN
619+
expect(nestedStackCount).toEqual(2);
614620
expect(deployedTemplate).toEqual({
615621
Resources: {
616622
NestedStack: {
@@ -781,9 +787,11 @@ test('readCurrentTemplateWithNestedStacks() succesfully ignores stacks without m
781787
));
782788

783789
// WHEN
784-
const deployedTemplate = await deployments.readCurrentTemplateWithNestedStacks(rootStack);
790+
const nestedStackCount = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).nestedStackCount;
791+
const deployedTemplate = (await deployments.readCurrentTemplateWithNestedStacks(rootStack)).deployedTemplate;
785792

786793
// THEN
794+
expect(nestedStackCount).toEqual(1);
787795
expect(deployedTemplate).toEqual({
788796
Resources: {
789797
WithMetadata: {

‎packages/aws-cdk/test/diff.test.ts

+76-27
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,15 @@ describe('non-nested stacks', () => {
5454
// Default implementations
5555
cloudFormation.readCurrentTemplateWithNestedStacks.mockImplementation((stackArtifact: CloudFormationStackArtifact) => {
5656
if (stackArtifact.stackName === 'D') {
57-
return Promise.resolve({ resource: 'D' });
57+
return Promise.resolve({
58+
deployedTemplate: { resource: 'D' },
59+
nestedStackCount: 0,
60+
});
5861
}
59-
return Promise.resolve({});
62+
return Promise.resolve({
63+
deployedTemplate: {},
64+
nestedStackCount: 0,
65+
});
6066
});
6167
cloudFormation.deployStack.mockImplementation((options) => Promise.resolve({
6268
noOp: true,
@@ -85,6 +91,43 @@ describe('non-nested stacks', () => {
8591
expect(exitCode).toBe(0);
8692
});
8793

94+
test('diff number of stack diffs, not resource diffs', async () => {
95+
// GIVEN
96+
cloudExecutable = new MockCloudExecutable({
97+
stacks: [{
98+
stackName: 'A',
99+
template: { resourceA: 'A', resourceB: 'B' },
100+
},
101+
{
102+
stackName: 'B',
103+
template: { resourceC: 'C' },
104+
}],
105+
});
106+
107+
toolkit = new CdkToolkit({
108+
cloudExecutable,
109+
deployments: cloudFormation,
110+
configuration: cloudExecutable.configuration,
111+
sdkProvider: cloudExecutable.sdkProvider,
112+
});
113+
114+
const buffer = new StringWritable();
115+
116+
// WHEN
117+
const exitCode = await toolkit.diff({
118+
stackNames: ['A', 'B'],
119+
stream: buffer,
120+
});
121+
122+
// THEN
123+
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
124+
expect(plainTextOutput).toContain('Stack A');
125+
expect(plainTextOutput).toContain('Stack B');
126+
127+
expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 2');
128+
expect(exitCode).toBe(0);
129+
});
130+
88131
test('exits with 1 with diffs and fail set to true', async () => {
89132
// GIVEN
90133
const buffer = new StringWritable();
@@ -161,7 +204,7 @@ describe('nested stacks', () => {
161204
cloudExecutable = new MockCloudExecutable({
162205
stacks: [{
163206
stackName: 'Parent',
164-
template: { },
207+
template: {},
165208
}],
166209
});
167210

@@ -209,41 +252,47 @@ describe('nested stacks', () => {
209252
},
210253
};
211254
return Promise.resolve({
212-
Resources: {
213-
AdditionChild: {
214-
Type: 'AWS::CloudFormation::Stack',
215-
Resources: {
216-
SomeResource: {
217-
Type: 'AWS::Something',
255+
deployedTemplate: {
256+
Resources: {
257+
AdditionChild: {
258+
Type: 'AWS::CloudFormation::Stack',
259+
Resources: {
260+
SomeResource: {
261+
Type: 'AWS::Something',
262+
},
218263
},
219264
},
220-
},
221-
DeletionChild: {
222-
Type: 'AWS::CloudFormation::Stack',
223-
Resources: {
224-
SomeResource: {
225-
Type: 'AWS::Something',
226-
Properties: {
227-
Prop: 'value-to-be-removed',
265+
DeletionChild: {
266+
Type: 'AWS::CloudFormation::Stack',
267+
Resources: {
268+
SomeResource: {
269+
Type: 'AWS::Something',
270+
Properties: {
271+
Prop: 'value-to-be-removed',
272+
},
228273
},
229274
},
230275
},
231-
},
232-
ChangedChild: {
233-
Type: 'AWS::CloudFormation::Stack',
234-
Resources: {
235-
SomeResource: {
236-
Type: 'AWS::Something',
237-
Properties: {
238-
Prop: 'old-value',
276+
ChangedChild: {
277+
Type: 'AWS::CloudFormation::Stack',
278+
Resources: {
279+
SomeResource: {
280+
Type: 'AWS::Something',
281+
Properties: {
282+
Prop: 'old-value',
283+
},
239284
},
240285
},
241286
},
242287
},
243288
},
289+
nestedStackCount: 3,
244290
});
245291
}
246-
return Promise.resolve({});
292+
return Promise.resolve({
293+
deployedTemplate: {},
294+
nestedStackCount: 0,
295+
});
247296
});
248297
});
249298

@@ -279,7 +328,7 @@ Resources
279328
└─ [+] new-value
280329
281330
282-
✨ Number of stacks with differences: 3`);
331+
✨ Number of stacks with differences: 4`);
283332

284333
expect(exitCode).toBe(0);
285334
});

0 commit comments

Comments
 (0)