Skip to content

Commit 9855c8f

Browse files
mrgrainmrgrain
authored
refactor(toolkit-lib): standardize confirmation requests to use ConfirmationRequest interface (#904)
This PR standardizes user confirmation flows across the CDK CLI by migrating from ad-hoc `promptly.confirm` usage to the structured `ConfirmationRequest` interface. These are the changes as suggested here: #826 (review) ## Changes - **Convert CDK_TOOLKIT_I8910**: Changed from `DataRequest` to `ConfirmationRequest` for refactor confirmation - **Standardize destroy flow**: Replace `promptly.confirm` with `IO.CDK_TOOLKIT_I7010` message - **Standardize deploy flow**: Update `askUserConfirmation` to use `ActionLessRequest<ConfirmationRequest>` pattern - **Clean up test infrastructure**: Remove deprecated `markTesting` function and `TESTING` variable - **Update tests**: Replace `promptly` mocks with `requestSpy` for consistency ## Benefits - Consistent user interaction patterns across all CLI commands - Better testability through standardized mocking - Improved error handling and TTY detection through the IO system - Cleaner separation of concerns between UI logic and business logic --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
1 parent 05954dd commit 9855c8f

File tree

6 files changed

+49
-60
lines changed

6 files changed

+49
-60
lines changed

‎packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/destroy/cdk-destroy-interactive.integtest.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ integTest('cdk destroy prompts the user for confirmation', withDefaultFixture(as
1414
interact: [
1515
{ prompt: /Are you sure you want to delete/, input: 'no' },
1616
],
17+
modEnv: {
18+
// disable coloring because it messes up prompt matching.
19+
FORCE_COLOR: '0',
20+
},
1721
});
1822

1923
// assert we didn't destroy the stack

‎packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import type {
2222
AssemblyData,
2323
ConfirmationRequest,
2424
ContextProviderMessageSource,
25-
DataRequest,
2625
Duration,
2726
ErrorPayload,
2827
SingleStack,
@@ -391,7 +390,7 @@ export const IO = {
391390
interface: 'RefactorResult',
392391
}),
393392

394-
CDK_TOOLKIT_I8910: make.question<DataRequest>({
393+
CDK_TOOLKIT_I8910: make.confirm<ConfirmationRequest>({
395394
code: 'CDK_TOOLKIT_I8910',
396395
description: 'Confirm refactor',
397396
interface: 'ConfirmationRequest',

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,10 +1190,9 @@ export class Toolkit extends CloudAssemblySourceBuilder {
11901190
}
11911191

11921192
const question = 'Do you wish to refactor these resources?';
1193-
const response = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I8910.req(question, {
1194-
responseDescription: '[Y]es/[n]o',
1195-
}, 'y'));
1196-
return ['y', 'yes'].includes(response.toLowerCase());
1193+
return ioHelper.requestResponse(IO.CDK_TOOLKIT_I8910.req(question, {
1194+
motivation: 'User input is needed',
1195+
}));
11971196
}
11981197

11991198
function formatError(error: any): string {

‎packages/@aws-cdk/toolkit-lib/test/actions/refactor.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -854,7 +854,7 @@ describe('refactor execution', () => {
854854
expect.objectContaining({
855855
action: 'refactor',
856856
code: 'CDK_TOOLKIT_I8910',
857-
defaultResponse: 'y',
857+
defaultResponse: true,
858858
level: 'info',
859859
message: 'Do you wish to refactor these resources?',
860860
}),

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

Lines changed: 34 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,17 @@ import * as path from 'path';
22
import { format } from 'util';
33
import { RequireApproval } from '@aws-cdk/cloud-assembly-schema';
44
import * as cxapi from '@aws-cdk/cx-api';
5-
import type { DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib';
5+
import type { ConfirmationRequest, DeploymentMethod, ToolkitAction, ToolkitOptions } from '@aws-cdk/toolkit-lib';
66
import { PermissionChangeType, Toolkit, ToolkitError } from '@aws-cdk/toolkit-lib';
77
import * as chalk from 'chalk';
88
import * as chokidar from 'chokidar';
99
import * as fs from 'fs-extra';
10-
import * as promptly from 'promptly';
1110
import * as uuid from 'uuid';
1211
import { CliIoHost } from './io-host';
1312
import type { Configuration } from './user-configuration';
1413
import { PROJECT_CONFIG } from './user-configuration';
15-
import type { IoHelper } from '../../lib/api-private';
16-
import { asIoHelper, cfnApi, tagsForStack } from '../../lib/api-private';
14+
import type { ActionLessRequest, IoHelper } from '../../lib/api-private';
15+
import { asIoHelper, cfnApi, IO, tagsForStack } from '../../lib/api-private';
1716
import type { AssetBuildNode, AssetPublishNode, Concurrency, StackNode, WorkGraph } from '../api';
1817
import {
1918
CloudWatchLogEventMonitor,
@@ -74,12 +73,6 @@ import type { ErrorDetails } from './telemetry/schema';
7473
// eslint-disable-next-line @typescript-eslint/no-require-imports,@typescript-eslint/consistent-type-imports
7574
const pLimit: typeof import('p-limit') = require('p-limit');
7675

77-
let TESTING = false;
78-
79-
export function markTesting() {
80-
TESTING = true;
81-
}
82-
8376
export interface CdkToolkitProps {
8477
/**
8578
* The Cloud Executable
@@ -495,12 +488,16 @@ export class CdkToolkit {
495488
});
496489
const securityDiff = formatter.formatSecurityDiff();
497490
if (requiresApproval(requireApproval, securityDiff.permissionChangeType)) {
491+
const motivation = '"--require-approval" is enabled and stack includes security-sensitive updates';
498492
await this.ioHost.asIoHelper().defaults.info(securityDiff.formattedDiff);
499493
await askUserConfirmation(
500494
this.ioHost,
501-
concurrency,
502-
'"--require-approval" is enabled and stack includes security-sensitive updates',
503-
'Do you wish to deploy these changes',
495+
IO.CDK_TOOLKIT_I5060.req(`${motivation}: 'Do you wish to deploy these changes'`, {
496+
motivation,
497+
concurrency,
498+
permissionChangeType: securityDiff.permissionChangeType,
499+
templateDiffs: formatter.diffs,
500+
}),
504501
);
505502
}
506503
}
@@ -578,9 +575,10 @@ export class CdkToolkit {
578575
} else {
579576
await askUserConfirmation(
580577
this.ioHost,
581-
concurrency,
582-
motivation,
583-
`${motivation}. Roll back first and then proceed with deployment`,
578+
IO.CDK_TOOLKIT_I5050.req(`${motivation}. Roll back first and then proceed with deployment`, {
579+
motivation,
580+
concurrency,
581+
}),
584582
);
585583
}
586584

@@ -604,9 +602,10 @@ export class CdkToolkit {
604602
} else {
605603
await askUserConfirmation(
606604
this.ioHost,
607-
concurrency,
608-
motivation,
609-
`${motivation}. Perform a regular deployment`,
605+
IO.CDK_TOOLKIT_I5050.req(`${motivation}. Perform a regular deployment`, {
606+
concurrency,
607+
motivation,
608+
}),
610609
);
611610
}
612611

@@ -970,33 +969,37 @@ export class CdkToolkit {
970969
}
971970

972971
public async destroy(options: DestroyOptions) {
973-
let stacks = await this.selectStacksForDestroy(options.selector, options.exclusively);
972+
const ioHelper = this.ioHost.asIoHelper();
974973

975974
// The stacks will have been ordered for deployment, so reverse them for deletion.
976-
stacks = stacks.reversed();
975+
const stacks = (await this.selectStacksForDestroy(options.selector, options.exclusively)).reversed();
977976

978977
if (!options.force) {
979-
// eslint-disable-next-line @stylistic/max-len
980-
const confirmed = await promptly.confirm(
981-
`Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))} (y/n)?`,
982-
);
983-
if (!confirmed) {
978+
const motivation = 'Destroying stacks is an irreversible action';
979+
const question = `Are you sure you want to delete: ${chalk.blue(stacks.stackArtifacts.map((s) => s.hierarchicalId).join(', '))}`;
980+
try {
981+
await ioHelper.requestResponse(IO.CDK_TOOLKIT_I7010.req(question, { motivation }));
982+
} catch (err: unknown) {
983+
if (!ToolkitError.isToolkitError(err) || err.message != 'Aborted by user') {
984+
throw err; // unexpected error
985+
}
986+
await ioHelper.notify(IO.CDK_TOOLKIT_E7010.msg(err.message));
984987
return;
985988
}
986989
}
987990

988991
const action = options.fromDeploy ? 'deploy' : 'destroy';
989992
for (const [index, stack] of stacks.stackArtifacts.entries()) {
990-
await this.ioHost.asIoHelper().defaults.info(chalk.green('%s: destroying... [%s/%s]'), chalk.blue(stack.displayName), index + 1, stacks.stackCount);
993+
await ioHelper.defaults.info(chalk.green('%s: destroying... [%s/%s]'), chalk.blue(stack.displayName), index + 1, stacks.stackCount);
991994
try {
992995
await this.props.deployments.destroyStack({
993996
stack,
994997
deployName: stack.stackName,
995998
roleArn: options.roleArn,
996999
});
997-
await this.ioHost.asIoHelper().defaults.info(chalk.green(`\n ✅ %s: ${action}ed`), chalk.blue(stack.displayName));
1000+
await ioHelper.defaults.info(chalk.green(`\n ✅ %s: ${action}ed`), chalk.blue(stack.displayName));
9981001
} catch (e) {
999-
await this.ioHost.asIoHelper().defaults.error(`\n ❌ %s: ${action} failed`, chalk.blue(stack.displayName), e);
1002+
await ioHelper.defaults.error(`\n ❌ %s: ${action} failed`, chalk.blue(stack.displayName), e);
10001003
throw e;
10011004
}
10021005
}
@@ -2103,25 +2106,10 @@ function buildParameterMap(
21032106
*/
21042107
async function askUserConfirmation(
21052108
ioHost: CliIoHost,
2106-
concurrency: number,
2107-
motivation: string,
2108-
question: string,
2109+
req: ActionLessRequest<ConfirmationRequest, boolean>,
21092110
) {
21102111
await ioHost.withCorkedLogging(async () => {
2111-
// only talk to user if STDIN is a terminal (otherwise, fail)
2112-
if (!TESTING && !process.stdin.isTTY) {
2113-
throw new ToolkitError(`${motivation}, but terminal (TTY) is not attached so we are unable to get a confirmation from the user`);
2114-
}
2115-
2116-
// only talk to user if concurrency is 1 (otherwise, fail)
2117-
if (concurrency > 1) {
2118-
throw new ToolkitError(`${motivation}, but concurrency is greater than 1 so we are unable to get a confirmation from the user`);
2119-
}
2120-
2121-
const confirmed = await promptly.confirm(`${chalk.cyan(question)} (y/n)?`);
2122-
if (!confirmed) {
2123-
throw new ToolkitError('Aborted by user');
2124-
}
2112+
await ioHost.asIoHelper().requestResponse(req);
21252113
});
21262114
}
21272115

‎packages/aws-cdk/test/cli/cdk-toolkit.test.ts‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ import type { DestroyStackResult } from '@aws-cdk/toolkit-lib/lib/api/deployment
6464
import { DescribeStacksCommand, GetTemplateCommand, StackStatus } from '@aws-sdk/client-cloudformation';
6565
import { GetParameterCommand } from '@aws-sdk/client-ssm';
6666
import * as fs from 'fs-extra';
67-
import * as promptly from 'promptly';
6867
import type { Template, SdkProvider } from '../../lib/api';
6968
import { Bootstrapper, type BootstrapSource } from '../../lib/api/bootstrap';
7069
import type {
@@ -81,7 +80,7 @@ import {
8180
import { Mode } from '../../lib/api/plugin';
8281
import type { Tag } from '../../lib/api/tags';
8382
import { asIoHelper } from '../../lib/api-private';
84-
import { CdkToolkit, markTesting } from '../../lib/cli/cdk-toolkit';
83+
import { CdkToolkit } from '../../lib/cli/cdk-toolkit';
8584
import { CliIoHost } from '../../lib/cli/io-host';
8685
import { Configuration } from '../../lib/cli/user-configuration';
8786
import { StackActivityProgress } from '../../lib/commands/deploy';
@@ -99,14 +98,13 @@ import {
9998
} from '../_helpers/mock-sdk';
10099
import { promiseWithResolvers } from '../_helpers/promises';
101100

102-
markTesting();
103-
104101
const defaultBootstrapSource: BootstrapSource = { source: 'default' };
105102
const bootstrapEnvironmentMock = jest.spyOn(Bootstrapper.prototype, 'bootstrapEnvironment');
106103
let cloudExecutable: MockCloudExecutable;
107104
let ioHost = CliIoHost.instance();
108105
let ioHelper = asIoHelper(ioHost, 'deploy');
109106
let notifySpy = jest.spyOn(ioHost, 'notify');
107+
let requestSpy = jest.spyOn(ioHost, 'requestResponse');
110108

111109
beforeEach(async () => {
112110
jest.resetAllMocks();
@@ -1700,7 +1698,8 @@ describe('rollback', () => {
17001698
stackArn: 'stack:arn',
17011699
});
17021700

1703-
const mockedConfirm = jest.spyOn(promptly, 'confirm').mockResolvedValue(true);
1701+
// respond with yes
1702+
requestSpy.mockImplementationOnce(async () => true);
17041703

17051704
const toolkit = new CdkToolkit({
17061705
ioHost,
@@ -1725,9 +1724,9 @@ describe('rollback', () => {
17251724
if (!useForce) {
17261725
// Questions will have been asked only if --force is not specified
17271726
if (firstResult.type === 'failpaused-need-rollback-first') {
1728-
expect(mockedConfirm).toHaveBeenCalledWith(expect.stringContaining('Roll back first and then proceed with deployment'));
1727+
expect(requestSpy).toHaveBeenCalledWith(expectIoMsg(expect.stringContaining('Roll back first and then proceed with deployment')));
17291728
} else {
1730-
expect(mockedConfirm).toHaveBeenCalledWith(expect.stringContaining('Perform a regular deployment'));
1729+
expect(requestSpy).toHaveBeenCalledWith(expectIoMsg(expect.stringContaining('Perform a regular deployment')));
17311730
}
17321731
}
17331732

0 commit comments

Comments
 (0)