Skip to content

Commit 6df13e8

Browse files
awsmadiawsmadi
committed
fix(cli): address PR review feedback for QuickSight hotswap
- Evaluate ImportMode through evaluateCfnExpression instead of passing raw CFN, consistent with how other properties are handled - Evaluate Name fallback through evaluateCfnExpression so CFN intrinsic functions (Ref, Fn::Join, etc.) resolve correctly even when Name itself hasn't changed - Remove Credentials from DataSource hotswappable properties — credential changes should go through CloudFormation for proper review - Add tests for CFN intrinsic functions on hotswappable properties - Add tests for CFN intrinsic functions in ImportMode (DataSet) - Add test for mixed hotswappable and non-hotswappable property changes - Add test verifying Credentials changes are not hotswapped
1 parent d8b42fd commit 6df13e8

File tree

2 files changed

+208
-8
lines changed

2 files changed

+208
-8
lines changed

‎packages/@aws-cdk/toolkit-lib/lib/api/hotswap/quicksight.ts‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import type { EvaluateCloudFormationTemplate } from '../cloudformation';
55

66
const QUICKSIGHT_RESOURCE_TYPES: Record<string, { hotswappableProps: string[]; service: string }> = {
77
'AWS::QuickSight::DataSet': { hotswappableProps: ['PhysicalTableMap', 'LogicalTableMap', 'Name'], service: 'quicksight-dataset' },
8-
'AWS::QuickSight::DataSource': { hotswappableProps: ['DataSourceParameters', 'Name', 'Credentials'], service: 'quicksight-datasource' },
8+
'AWS::QuickSight::DataSource': { hotswappableProps: ['DataSourceParameters', 'Name'], service: 'quicksight-datasource' },
99
'AWS::QuickSight::Dashboard': { hotswappableProps: ['Definition', 'Name', 'SourceEntity'], service: 'quicksight-dashboard' },
1010
'AWS::QuickSight::Analysis': { hotswappableProps: ['Definition', 'Name', 'SourceEntity'], service: 'quicksight-analysis' },
1111
'AWS::QuickSight::Template': { hotswappableProps: ['Definition', 'Name', 'SourceEntity'], service: 'quicksight-template' },
@@ -68,31 +68,36 @@ export async function isHotswappableQuickSightChange(
6868
}
6969
}
7070

71+
const evaluatedName = props.Name !== undefined
72+
? (evaluatedProps.Name ?? await evaluateCfnTemplate.evaluateCfnExpression(props.Name))
73+
: undefined;
74+
7175
switch (change.newValue.Type) {
7276
case 'AWS::QuickSight::DataSet':
7377
await sdk.quickSight().updateDataSet({
7478
AwsAccountId: awsAccountId,
7579
DataSetId: resourceId,
76-
Name: evaluatedProps.Name ?? props.Name,
80+
Name: evaluatedName,
7781
PhysicalTableMap: evaluatedProps.PhysicalTableMap,
7882
LogicalTableMap: evaluatedProps.LogicalTableMap,
79-
ImportMode: props.ImportMode,
83+
ImportMode: props.ImportMode !== undefined
84+
? await evaluateCfnTemplate.evaluateCfnExpression(props.ImportMode)
85+
: undefined,
8086
});
8187
break;
8288
case 'AWS::QuickSight::DataSource':
8389
await sdk.quickSight().updateDataSource({
8490
AwsAccountId: awsAccountId,
8591
DataSourceId: resourceId,
86-
Name: evaluatedProps.Name ?? props.Name,
92+
Name: evaluatedName,
8793
DataSourceParameters: evaluatedProps.DataSourceParameters,
88-
Credentials: evaluatedProps.Credentials,
8994
});
9095
break;
9196
case 'AWS::QuickSight::Dashboard':
9297
await sdk.quickSight().updateDashboard({
9398
AwsAccountId: awsAccountId,
9499
DashboardId: resourceId,
95-
Name: evaluatedProps.Name ?? props.Name,
100+
Name: evaluatedName,
96101
Definition: evaluatedProps.Definition,
97102
SourceEntity: evaluatedProps.SourceEntity,
98103
});
@@ -101,7 +106,7 @@ export async function isHotswappableQuickSightChange(
101106
await sdk.quickSight().updateAnalysis({
102107
AwsAccountId: awsAccountId,
103108
AnalysisId: resourceId,
104-
Name: evaluatedProps.Name ?? props.Name,
109+
Name: evaluatedName,
105110
Definition: evaluatedProps.Definition,
106111
SourceEntity: evaluatedProps.SourceEntity,
107112
});
@@ -110,7 +115,7 @@ export async function isHotswappableQuickSightChange(
110115
await sdk.quickSight().updateTemplate({
111116
AwsAccountId: awsAccountId,
112117
TemplateId: resourceId,
113-
Name: evaluatedProps.Name ?? props.Name,
118+
Name: evaluatedName,
114119
Definition: evaluatedProps.Definition,
115120
SourceEntity: evaluatedProps.SourceEntity,
116121
});

‎packages/@aws-cdk/toolkit-lib/test/api/hotswap/quicksight-hotswap-deployments.test.ts‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,201 @@ describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hot
240240
});
241241
});
242242

243+
test('correctly evaluates CFN intrinsic functions in hotswappable properties', async () => {
244+
setup.setCurrentCfnStackTemplate({
245+
Resources: {
246+
Bucket: {
247+
Type: 'AWS::S3::Bucket',
248+
},
249+
Dashboard: {
250+
Type: 'AWS::QuickSight::Dashboard',
251+
Properties: {
252+
AwsAccountId: '123456789012',
253+
DashboardId: 'my-dashboard',
254+
Name: {
255+
'Fn::Join': ['-', ['dashboard', { Ref: 'Bucket' }]],
256+
},
257+
Definition: { old: {} },
258+
},
259+
},
260+
},
261+
});
262+
setup.pushStackResourceSummaries(setup.stackSummaryOf('Bucket', 'AWS::S3::Bucket', 'my-bucket'));
263+
const cdkStackArtifact = setup.cdkStackArtifactOf({
264+
template: {
265+
Resources: {
266+
Bucket: {
267+
Type: 'AWS::S3::Bucket',
268+
},
269+
Dashboard: {
270+
Type: 'AWS::QuickSight::Dashboard',
271+
Properties: {
272+
AwsAccountId: '123456789012',
273+
DashboardId: 'my-dashboard',
274+
Name: {
275+
'Fn::Join': ['-', ['dashboard', { Ref: 'Bucket' }]],
276+
},
277+
Definition: { new: {} },
278+
},
279+
},
280+
},
281+
},
282+
});
283+
284+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
285+
286+
expect(deployStackResult).not.toBeUndefined();
287+
expect(mockQuickSightClient).toHaveReceivedCommandWith(UpdateDashboardCommand, {
288+
AwsAccountId: '123456789012',
289+
DashboardId: 'my-dashboard',
290+
Name: 'dashboard-my-bucket',
291+
Definition: { new: {} },
292+
});
293+
});
294+
295+
test('correctly evaluates CFN intrinsic functions in ImportMode for DataSet', async () => {
296+
setup.setCurrentCfnStackTemplate({
297+
Resources: {
298+
Param: {
299+
Type: 'AWS::SSM::Parameter',
300+
},
301+
DataSet: {
302+
Type: 'AWS::QuickSight::DataSet',
303+
Properties: {
304+
AwsAccountId: '123456789012',
305+
DataSetId: 'my-dataset',
306+
Name: 'MyDataSet',
307+
ImportMode: { Ref: 'Param' },
308+
PhysicalTableMap: { old: {} },
309+
},
310+
},
311+
},
312+
});
313+
setup.pushStackResourceSummaries(setup.stackSummaryOf('Param', 'AWS::SSM::Parameter', 'SPICE'));
314+
const cdkStackArtifact = setup.cdkStackArtifactOf({
315+
template: {
316+
Resources: {
317+
Param: {
318+
Type: 'AWS::SSM::Parameter',
319+
},
320+
DataSet: {
321+
Type: 'AWS::QuickSight::DataSet',
322+
Properties: {
323+
AwsAccountId: '123456789012',
324+
DataSetId: 'my-dataset',
325+
Name: 'MyDataSet',
326+
ImportMode: { Ref: 'Param' },
327+
PhysicalTableMap: { new: {} },
328+
},
329+
},
330+
},
331+
},
332+
});
333+
334+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
335+
336+
expect(deployStackResult).not.toBeUndefined();
337+
expect(mockQuickSightClient).toHaveReceivedCommandWith(UpdateDataSetCommand, {
338+
AwsAccountId: '123456789012',
339+
DataSetId: 'my-dataset',
340+
Name: 'MyDataSet',
341+
PhysicalTableMap: { new: {} },
342+
ImportMode: 'SPICE',
343+
});
344+
});
345+
346+
test('hotswaps hotswappable properties and reports non-hotswappable properties when both change together', async () => {
347+
setup.setCurrentCfnStackTemplate({
348+
Resources: {
349+
DataSet: {
350+
Type: 'AWS::QuickSight::DataSet',
351+
Properties: {
352+
AwsAccountId: '123456789012',
353+
DataSetId: 'my-dataset',
354+
Name: 'MyDataSet',
355+
ImportMode: 'SPICE',
356+
PhysicalTableMap: { old: {} },
357+
Permissions: [{ Principal: 'old-principal' }],
358+
},
359+
},
360+
},
361+
});
362+
const cdkStackArtifact = setup.cdkStackArtifactOf({
363+
template: {
364+
Resources: {
365+
DataSet: {
366+
Type: 'AWS::QuickSight::DataSet',
367+
Properties: {
368+
AwsAccountId: '123456789012',
369+
DataSetId: 'my-dataset',
370+
Name: 'MyDataSet',
371+
ImportMode: 'SPICE',
372+
PhysicalTableMap: { new: {} },
373+
Permissions: [{ Principal: 'new-principal' }],
374+
},
375+
},
376+
},
377+
},
378+
});
379+
380+
if (hotswapMode === HotswapMode.FALL_BACK) {
381+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
382+
expect(deployStackResult).toBeUndefined();
383+
expect(mockQuickSightClient).not.toHaveReceivedCommand(UpdateDataSetCommand);
384+
} else {
385+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
386+
expect(deployStackResult).not.toBeUndefined();
387+
expect(mockQuickSightClient).toHaveReceivedCommandWith(UpdateDataSetCommand, {
388+
AwsAccountId: '123456789012',
389+
DataSetId: 'my-dataset',
390+
Name: 'MyDataSet',
391+
PhysicalTableMap: { new: {} },
392+
ImportMode: 'SPICE',
393+
});
394+
}
395+
});
396+
397+
test('does not hotswap Credentials changes on DataSource', async () => {
398+
setup.setCurrentCfnStackTemplate({
399+
Resources: {
400+
DataSource: {
401+
Type: 'AWS::QuickSight::DataSource',
402+
Properties: {
403+
AwsAccountId: '123456789012',
404+
DataSourceId: 'my-datasource',
405+
Name: 'MyDataSource',
406+
Credentials: { CredentialPair: { Username: 'old-user', Password: 'old-pass' } },
407+
},
408+
},
409+
},
410+
});
411+
const cdkStackArtifact = setup.cdkStackArtifactOf({
412+
template: {
413+
Resources: {
414+
DataSource: {
415+
Type: 'AWS::QuickSight::DataSource',
416+
Properties: {
417+
AwsAccountId: '123456789012',
418+
DataSourceId: 'my-datasource',
419+
Name: 'MyDataSource',
420+
Credentials: { CredentialPair: { Username: 'new-user', Password: 'new-pass' } },
421+
},
422+
},
423+
},
424+
},
425+
});
426+
427+
if (hotswapMode === HotswapMode.FALL_BACK) {
428+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
429+
expect(deployStackResult).toBeUndefined();
430+
} else {
431+
const deployStackResult = await hotswapMockSdkProvider.tryHotswapDeployment(hotswapMode, cdkStackArtifact);
432+
expect(deployStackResult).not.toBeUndefined();
433+
expect(deployStackResult?.noOp).toEqual(true);
434+
}
435+
expect(mockQuickSightClient).not.toHaveReceivedCommand(UpdateDataSourceCommand);
436+
});
437+
243438
test('does not hotswap when non-hotswappable property changes', async () => {
244439
setup.setCurrentCfnStackTemplate({
245440
Resources: {

0 commit comments

Comments
 (0)