From c3c3057eae9a7b008ad5585996a2a9e17d1e5dac Mon Sep 17 00:00:00 2001 From: Roshane Pascual Date: Tue, 29 Oct 2024 17:01:10 -0700 Subject: [PATCH] update ctrl+c behavior to always print guidance and not prompt (#2160) * update ctrl+c behavior to always print guidance and not prompt * pr feedback * Update .changeset/bright-jokes-draw.md Co-authored-by: Kamil Sobol --------- Co-authored-by: Kamil Sobol --- .changeset/bright-jokes-draw.md | 6 + .../package_manager_controller_base.ts | 1 + .../pnpm_package_manager_controller.ts | 6 - ...yarn_classic_package_manager_controller.ts | 6 - .../sandbox_delete_command.test.ts | 10 +- .../commands/sandbox/sandbox_command.test.ts | 123 +----------------- .../src/commands/sandbox/sandbox_command.ts | 26 +--- .../sandbox/sandbox_command_factory.ts | 7 +- 8 files changed, 18 insertions(+), 167 deletions(-) create mode 100644 .changeset/bright-jokes-draw.md diff --git a/.changeset/bright-jokes-draw.md b/.changeset/bright-jokes-draw.md new file mode 100644 index 0000000000..9a3649dfde --- /dev/null +++ b/.changeset/bright-jokes-draw.md @@ -0,0 +1,6 @@ +--- +'@aws-amplify/cli-core': minor +'@aws-amplify/backend-cli': minor +--- + +update ctrl+c behavior to always print guidance to delete and exit with no prompt diff --git a/packages/cli-core/src/package-manager-controller/package_manager_controller_base.ts b/packages/cli-core/src/package-manager-controller/package_manager_controller_base.ts index d722ef84f5..4d9786b391 100644 --- a/packages/cli-core/src/package-manager-controller/package_manager_controller_base.ts +++ b/packages/cli-core/src/package-manager-controller/package_manager_controller_base.ts @@ -137,6 +137,7 @@ export abstract class PackageManagerControllerBase /** * allowsSignalPropagation - Determines if the package manager allows the process * signals such as SIGINT to be propagated to the underlying node process. + * @deprecated */ allowsSignalPropagation = () => true; diff --git a/packages/cli-core/src/package-manager-controller/pnpm_package_manager_controller.ts b/packages/cli-core/src/package-manager-controller/pnpm_package_manager_controller.ts index 321ad9cd26..5eb13ea7de 100644 --- a/packages/cli-core/src/package-manager-controller/pnpm_package_manager_controller.ts +++ b/packages/cli-core/src/package-manager-controller/pnpm_package_manager_controller.ts @@ -32,10 +32,4 @@ export class PnpmPackageManagerController extends PackageManagerControllerBase { existsSync ); } - - /** - * Pnpm doesn't handle the node process gracefully during the SIGINT life cycle. - * See: https://github.com/pnpm/pnpm/issues/7374 - */ - allowsSignalPropagation = () => false; } diff --git a/packages/cli-core/src/package-manager-controller/yarn_classic_package_manager_controller.ts b/packages/cli-core/src/package-manager-controller/yarn_classic_package_manager_controller.ts index 2f7a837e76..48a4330ed9 100644 --- a/packages/cli-core/src/package-manager-controller/yarn_classic_package_manager_controller.ts +++ b/packages/cli-core/src/package-manager-controller/yarn_classic_package_manager_controller.ts @@ -37,12 +37,6 @@ export class YarnClassicPackageManagerController extends PackageManagerControlle await this.addTypescript(targetDir); await super.initializeTsConfig(targetDir); }; - /** - * - * Yarn doesn't respect the SIGINT life cycle and exits immediately leaving - * the node process hanging. See: https://github.com/yarnpkg/yarn/issues/8895 - */ - allowsSignalPropagation = () => false; private addTypescript = async (targetDir: string) => { await this.executeWithDebugLogger( diff --git a/packages/cli/src/commands/sandbox/sandbox-delete/sandbox_delete_command.test.ts b/packages/cli/src/commands/sandbox/sandbox-delete/sandbox_delete_command.test.ts index 62e80f946c..c919cd6141 100644 --- a/packages/cli/src/commands/sandbox/sandbox-delete/sandbox_delete_command.test.ts +++ b/packages/cli/src/commands/sandbox/sandbox-delete/sandbox_delete_command.test.ts @@ -1,10 +1,5 @@ import { beforeEach, describe, it, mock } from 'node:test'; -import { - AmplifyPrompter, - PackageManagerControllerFactory, - format, - printer, -} from '@aws-amplify/cli-core'; +import { AmplifyPrompter, format, printer } from '@aws-amplify/cli-core'; import yargs, { CommandModule } from 'yargs'; import { TestCommandRunner } from '../../../test-utils/command_runner.js'; import assert from 'node:assert'; @@ -50,8 +45,7 @@ void describe('sandbox delete command', () => { sandboxFactory, [sandboxDeleteCommand, createSandboxSecretCommand()], clientConfigGeneratorAdapterMock, - commandMiddleware, - new PackageManagerControllerFactory().getPackageManagerController() + commandMiddleware ); const parser = yargs().command(sandboxCommand as unknown as CommandModule); commandRunner = new TestCommandRunner(parser); diff --git a/packages/cli/src/commands/sandbox/sandbox_command.test.ts b/packages/cli/src/commands/sandbox/sandbox_command.test.ts index ce5e2ca957..6d7df80f9b 100644 --- a/packages/cli/src/commands/sandbox/sandbox_command.test.ts +++ b/packages/cli/src/commands/sandbox/sandbox_command.test.ts @@ -8,7 +8,7 @@ import { TestCommandError, TestCommandRunner, } from '../../test-utils/command_runner.js'; -import { AmplifyPrompter, format, printer } from '@aws-amplify/cli-core'; +import { format, printer } from '@aws-amplify/cli-core'; import { EventHandler, SandboxCommand } from './sandbox_command.js'; import { createSandboxCommand } from './sandbox_command_factory.js'; import { SandboxDeleteCommand } from './sandbox-delete/sandbox_delete_command.js'; @@ -20,7 +20,6 @@ import { import { createSandboxSecretCommand } from './sandbox-secret/sandbox_secret_command_factory.js'; import { ClientConfigGeneratorAdapter } from '../../client-config/client_config_generator_adapter.js'; import { CommandMiddleware } from '../../command_middleware.js'; -import { PackageManagerController } from '@aws-amplify/plugin-types'; import { AmplifyError } from '@aws-amplify/platform-core'; mock.method(fsp, 'mkdir', () => Promise.resolve()); @@ -54,11 +53,6 @@ void describe('sandbox command', () => { ); const sandboxProfile = 'test-sandbox'; - const allowsSignalPropagationMock = mock.fn(() => true); - const packageManagerControllerMock = { - allowsSignalPropagation: allowsSignalPropagationMock, - } as unknown as PackageManagerController; - beforeEach(async () => { const sandboxFactory = new SandboxSingletonFactory( () => @@ -80,7 +74,6 @@ void describe('sandbox command', () => { [sandboxDeleteCommand, createSandboxSecretCommand()], clientConfigGeneratorAdapterMock, commandMiddleware, - packageManagerControllerMock, () => ({ successfulDeployment: [clientConfigGenerationMock], successfulDeletion: [clientConfigDeletionMock], @@ -189,118 +182,7 @@ void describe('sandbox command', () => { ); }); - void it('asks to delete the sandbox environment when users send ctrl-C and say yes to delete', async (contextual) => { - // Mock process and extract the sigint handler after calling the sandbox command - const processSignal = contextual.mock.method(process, 'on', () => { - /* no op */ - }); - const sandboxStartMock = contextual.mock.method( - sandbox, - 'start', - async () => Promise.resolve() - ); - - const sandboxDeleteMock = contextual.mock.method(sandbox, 'delete', () => - Promise.resolve() - ); - - // User said yes to delete - contextual.mock.method(AmplifyPrompter, 'yesOrNo', () => - Promise.resolve(true) - ); - - await commandRunner.runCommand('sandbox'); - - // Similar to the later 0ms timeout. Without this tests in github action are failing - // but working locally - await new Promise((resolve) => setTimeout(resolve, 0)); - const sigIntHandlerFn = processSignal.mock.calls[0].arguments[1]; - if (sigIntHandlerFn) sigIntHandlerFn(); - - // I can't find any open node:test or yargs issues that would explain why this is necessary - // but for some reason the mock call count does not update without this 0ms wait - await new Promise((resolve) => setTimeout(resolve, 0)); - assert.equal(sandboxStartMock.mock.callCount(), 1); - assert.equal(sandboxDeleteMock.mock.callCount(), 1); - }); - - void it('asks to delete the sandbox environment when users send ctrl-C and say yes to delete with profile', async (contextual) => { - // Mock process and extract the sigint handler after calling the sandbox command - const processSignal = contextual.mock.method(process, 'on', () => { - /* no op */ - }); - const sandboxStartMock = contextual.mock.method( - sandbox, - 'start', - async () => Promise.resolve() - ); - - const sandboxDeleteMock = contextual.mock.method(sandbox, 'delete', () => - Promise.resolve() - ); - - // User said yes to delete - contextual.mock.method(AmplifyPrompter, 'yesOrNo', () => - Promise.resolve(true) - ); - - const profile = 'test_profile'; - await commandRunner.runCommand(`sandbox --profile ${profile}`); - - // Similar to the later 0ms timeout. Without this tests in github action are failing - // but working locally - await new Promise((resolve) => setTimeout(resolve, 0)); - const sigIntHandlerFn = processSignal.mock.calls[0].arguments[1]; - if (sigIntHandlerFn) sigIntHandlerFn(); - - // I can't find any open node:test or yargs issues that would explain why this is necessary - // but for some reason the mock call count does not update without this 0ms wait - await new Promise((resolve) => setTimeout(resolve, 0)); - assert.equal(sandboxStartMock.mock.callCount(), 1); - assert.equal(sandboxDeleteMock.mock.callCount(), 1); - assert.deepStrictEqual(sandboxDeleteMock.mock.calls[0].arguments[0], { - identifier: undefined, - profile, - }); - }); - - void it('asks to delete the sandbox environment when users send ctrl-C and say no to delete', async (contextual) => { - // Mock process and extract the sigint handler after calling the sandbox command - const processSignal = contextual.mock.method(process, 'on', () => { - /* no op */ - }); - const sandboxStartMock = contextual.mock.method( - sandbox, - 'start', - async () => Promise.resolve() - ); - - const sandboxDeleteMock = contextual.mock.method( - sandbox, - 'delete', - async () => Promise.resolve() - ); - - // User said no to delete - contextual.mock.method(AmplifyPrompter, 'yesOrNo', () => - Promise.resolve(false) - ); - - await commandRunner.runCommand('sandbox'); - - // Similar to the previous test's 0ms timeout. Without this tests in github action are failing - // but working locally - await new Promise((resolve) => setTimeout(resolve, 0)); - const sigIntHandlerFn = processSignal.mock.calls[0].arguments[1]; - if (sigIntHandlerFn) sigIntHandlerFn(); - - assert.equal(sandboxStartMock.mock.callCount(), 1); - assert.equal(sandboxDeleteMock.mock.callCount(), 0); - }); - - void it('Does not prompt for deleting the sandbox if package manager does not allow signal propagation', async (contextual) => { - allowsSignalPropagationMock.mock.mockImplementationOnce(() => false); - + void it('Prints stopping sandbox and instructions to delete sandbox when users send ctrl+c', async (contextual) => { // Mock process and extract the sigint handler after calling the sandbox command const processSignal = contextual.mock.method(process, 'on', () => { /* no op */ @@ -371,7 +253,6 @@ void describe('sandbox command', () => { [], clientConfigGeneratorAdapterMock, commandMiddleware, - packageManagerControllerMock, undefined ); const parser = yargs().command(sandboxCommand as unknown as CommandModule); diff --git a/packages/cli/src/commands/sandbox/sandbox_command.ts b/packages/cli/src/commands/sandbox/sandbox_command.ts index 356d0a8eb6..37c069286b 100644 --- a/packages/cli/src/commands/sandbox/sandbox_command.ts +++ b/packages/cli/src/commands/sandbox/sandbox_command.ts @@ -1,7 +1,7 @@ import { ArgumentsCamelCase, Argv, CommandModule } from 'yargs'; import fs from 'fs'; import fsp from 'fs/promises'; -import { AmplifyPrompter, format, printer } from '@aws-amplify/cli-core'; +import { format, printer } from '@aws-amplify/cli-core'; import { SandboxFunctionStreamingOptions, SandboxSingletonFactory, @@ -20,7 +20,6 @@ import { ClientConfigGeneratorAdapter } from '../../client-config/client_config_ import { CommandMiddleware } from '../../command_middleware.js'; import { SandboxCommandGlobalOptions } from './option_types.js'; import { ArgumentsKebabCase } from '../../kebab_case.js'; -import { PackageManagerController } from '@aws-amplify/plugin-types'; import { AmplifyUserError } from '@aws-amplify/platform-core'; export type SandboxCommandOptionsKebabCase = ArgumentsKebabCase< @@ -81,7 +80,6 @@ export class SandboxCommand private readonly sandboxSubCommands: CommandModule[], private clientConfigGeneratorAdapter: ClientConfigGeneratorAdapter, private commandMiddleware: CommandMiddleware, - private readonly packageManagerController: PackageManagerController, private readonly sandboxEventHandlerCreator?: SandboxEventHandlerCreator ) { this.command = 'sandbox'; @@ -276,23 +274,11 @@ export class SandboxCommand }; sigIntHandler = async () => { - if (!this.packageManagerController.allowsSignalPropagation()) { - printer.print( - `Stopping the sandbox process. To delete the sandbox, run ${format.normalizeAmpxCommand( - 'sandbox delete' - )}` - ); - return; - } - const answer = await AmplifyPrompter.yesOrNo({ - message: - 'Would you like to delete all the resources in your sandbox environment (This cannot be undone)?', - defaultValue: false, - }); - if (answer) - await ( - await this.sandboxFactory.getInstance() - ).delete({ identifier: this.sandboxIdentifier, profile: this.profile }); + printer.print( + `Stopping the sandbox process. To delete the sandbox, run ${format.normalizeAmpxCommand( + 'sandbox delete' + )}` + ); }; private validateDirectory = async (option: string, dir: string) => { diff --git a/packages/cli/src/commands/sandbox/sandbox_command_factory.ts b/packages/cli/src/commands/sandbox/sandbox_command_factory.ts index 07afc979d0..ef5454a931 100644 --- a/packages/cli/src/commands/sandbox/sandbox_command_factory.ts +++ b/packages/cli/src/commands/sandbox/sandbox_command_factory.ts @@ -16,11 +16,7 @@ import { } from '@aws-amplify/platform-core'; import { SandboxEventHandlerFactory } from './sandbox_event_handler_factory.js'; import { CommandMiddleware } from '../../command_middleware.js'; -import { - PackageManagerControllerFactory, - format, - printer, -} from '@aws-amplify/cli-core'; +import { format, printer } from '@aws-amplify/cli-core'; import { S3Client } from '@aws-sdk/client-s3'; import { AmplifyClient } from '@aws-sdk/client-amplify'; import { CloudFormationClient } from '@aws-sdk/client-cloudformation'; @@ -70,7 +66,6 @@ export const createSandboxCommand = (): CommandModule< [new SandboxDeleteCommand(sandboxFactory), createSandboxSecretCommand()], clientConfigGeneratorAdapter, commandMiddleWare, - new PackageManagerControllerFactory().getPackageManagerController(), eventHandlerFactory.getSandboxEventHandlers ); };