chore(toolkit): fix various inconsistenncies with hotswap settings (#33240)

### Description of changes

- Make `ExtendedDeployOptions` properly private
- Don't print warnings if hotswap mode is not provided
- default `watch` action to `HOTSWAP_ONLY`
- fix `watch` user-agent

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

updated tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: mrgrain

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts

@@ -175,22 +175,6 @@ export interface BaseDeployOptions {
   readonly concurrency?: number;
 }
 
-/**
- * Deploy options needed by the watch command.
- * Intentionally not exported because these options are not
- * meant to be public facing.
- *
- * @internal
- */
-export interface ExtendedDeployOptions extends DeployOptions {
-  /**
-   * The extra string to append to the User-Agent header when performing AWS SDK calls.
-   *
-   * @default - nothing extra is appended to the User-Agent header
-   */
-  readonly extraUserAgent?: string;
-}
-
 export interface DeployOptions extends BaseDeployOptions {
   /**
    * ARNs of SNS topics that CloudFormation will notify with stack related events

packages/@aws-cdk/toolkit/lib/actions/deploy/private/index.ts

@@ -1 +1,17 @@
+import { DeployOptions } from '..';
+
 export * from './helpers';
+
+/**
+ * Deploy options needed by the watch command.
+ * Intentionally not exported because these options are not
+ * meant to be public facing.
+ */
+export interface ExtendedDeployOptions extends DeployOptions {
+  /**
+   * The extra string to append to the User-Agent header when performing AWS SDK calls.
+   *
+   * @default - nothing extra is appended to the User-Agent header
+   */
+  readonly extraUserAgent?: string;
+}

packages/@aws-cdk/toolkit/lib/actions/watch/index.ts

@@ -1,4 +1,4 @@
-import type { BaseDeployOptions } from '../deploy';
+import type { BaseDeployOptions, HotswapMode } from '../deploy';
 
 export interface WatchOptions extends BaseDeployOptions {
   /**
@@ -45,6 +45,17 @@ export interface WatchOptions extends BaseDeployOptions {
    * @default 'cdk.out'
    */
   readonly outdir?: string;
+
+  /**
+   * @TODO can this be part of `DeploymentMethod`
+   *
+   * Whether to perform a 'hotswap' deployment.
+   * A 'hotswap' deployment will attempt to short-circuit CloudFormation
+   * and update the affected resources like Lambda functions directly.
+   *
+   * @default HotswapMode.HOTSWAP_ONLY
+   */
+  readonly hotswap?: HotswapMode;
 }
 
 export function patternsArrayForWatch(

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

@@ -4,16 +4,16 @@ import * as chalk from 'chalk';
 import * as chokidar from 'chokidar';
 import * as fs from 'fs-extra';
 import { ToolkitServices } from './private';
-import { AssetBuildTime, DeployOptions, ExtendedDeployOptions, RequireApproval } from '../actions/deploy';
-import { buildParameterMap, removePublishedAssets } from '../actions/deploy/private';
-import { DestroyOptions } from '../actions/destroy';
-import { DiffOptions } from '../actions/diff';
+import { AssetBuildTime, type DeployOptions, RequireApproval } from '../actions/deploy';
+import { type ExtendedDeployOptions, buildParameterMap, removePublishedAssets } from '../actions/deploy/private';
+import { type DestroyOptions } from '../actions/destroy';
+import { type DiffOptions } from '../actions/diff';
 import { diffRequiresApproval } from '../actions/diff/private';
-import { ListOptions } from '../actions/list';
-import { RollbackOptions } from '../actions/rollback';
-import { SynthOptions } from '../actions/synth';
+import { type ListOptions } from '../actions/list';
+import { type RollbackOptions } from '../actions/rollback';
+import { type SynthOptions } from '../actions/synth';
 import { patternsArrayForWatch, WatchOptions } from '../actions/watch';
-import { SdkOptions } from '../api/aws-auth';
+import { type SdkOptions } from '../api/aws-auth';
 import { DEFAULT_TOOLKIT_STACK_NAME, SdkProvider, SuccessfulDeployStackResult, StackCollection, Deployments, HotswapMode, StackActivityProgress, ResourceMigrator, obscureTemplate, serializeStructure, tagsForStack, CliIoHost, validateSnsTopicArn, Concurrency, WorkGraphBuilder, AssetBuildNode, AssetPublishNode, StackNode, formatErrorMessage } from '../api/aws-cdk';
 import { CachedCloudAssemblySource, IdentityCloudAssemblySource, StackAssembly, ICloudAssemblySource, StackSelectionStrategy } from '../api/cloud-assembly';
 import { ALL_STACKS, CloudAssemblySourceBuilder } from '../api/cloud-assembly/private';
@@ -234,11 +234,12 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
 
     const parameterMap = buildParameterMap(options.parameters?.parameters);
 
-    if (options.hotswap !== HotswapMode.FULL_DEPLOYMENT) {
-      await ioHost.notify(warn(
+    const hotswapMode = options.hotswap ?? HotswapMode.FULL_DEPLOYMENT;
+    if (hotswapMode !== HotswapMode.FULL_DEPLOYMENT) {
+      await ioHost.notify(warn([
         '⚠️ The --hotswap and --hotswap-fallback flags deliberately introduce CloudFormation drift to speed up deployments',
-      ));
-      await ioHost.notify(warn('⚠️ They should only be used for development - never use them for your production Stacks!\n'));
+        '⚠️ They should only be used for development - never use them for your production Stacks!',
+      ].join('\n')));
     }
 
     // @TODO
@@ -367,7 +368,7 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
             progress,
             ci: options.ci,
             rollback,
-            hotswap: options.hotswap,
+            hotswap: hotswapMode,
             extraUserAgent: options.extraUserAgent,
             // hotswapPropertyOverrides: hotswapPropertyOverrides,
             assetParallelism: options.assetParallelism,
@@ -769,21 +770,18 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
     assembly: StackAssembly,
     options: WatchOptions,
   ): Promise<void> {
+    // watch defaults hotswap to enabled
+    const hotswap = options.hotswap ?? HotswapMode.HOTSWAP_ONLY;
     const deployOptions: ExtendedDeployOptions = {
       ...options,
       requireApproval: RequireApproval.NEVER,
       // cloudWatchLogMonitor,
-      hotswap: options.hotswap,
-      extraUserAgent: `cdk-watch/hotswap-${options.hotswap !== HotswapMode.FALL_BACK ? 'on' : 'off'}`,
-      concurrency: options.concurrency,
+      hotswap,
+      extraUserAgent: `cdk-watch/hotswap-${hotswap === HotswapMode.FULL_DEPLOYMENT ? 'off' : 'on'}`,
     };
 
     try {
-      await this._deploy(
-        assembly,
-        'watch',
-        deployOptions,
-      );
+      await this._deploy(assembly, 'watch', deployOptions);
     } catch {
       // just continue - deploy will show the error
     }

packages/@aws-cdk/toolkit/test/actions/deploy-hotswap.test.ts

@@ -0,0 +1,91 @@
+import { HotswapMode } from '../../lib';
+import { Toolkit } from '../../lib/toolkit';
+import { builderFixture, TestIoHost } from '../_helpers';
+
+const ioHost = new TestIoHost();
+const toolkit = new Toolkit({ ioHost });
+
+let mockDeployStack = jest.fn().mockResolvedValue({
+  type: 'did-deploy-stack',
+  stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack',
+  outputs: {},
+  noOp: false,
+});
+
+jest.mock('../../lib/api/aws-cdk', () => {
+  return {
+    ...jest.requireActual('../../lib/api/aws-cdk'),
+    Deployments: jest.fn().mockImplementation(() => ({
+      deployStack: mockDeployStack,
+      resolveEnvironment: jest.fn().mockResolvedValue({}),
+      isSingleAssetPublished: jest.fn().mockResolvedValue(true),
+      readCurrentTemplate: jest.fn().mockResolvedValue({ Resources: {} }),
+    })),
+  };
+});
+
+beforeEach(() => {
+  ioHost.notifySpy.mockClear();
+  ioHost.requestSpy.mockClear();
+  jest.clearAllMocks();
+});
+
+describe('deploy with hotswap', () => {
+  test('does print hotswap warnings for FALL_BACK mode', async () => {
+    // WHEN
+    const cx = await builderFixture(toolkit, 'two-empty-stacks');
+    await toolkit.deploy(cx, {
+      hotswap: HotswapMode.FALL_BACK,
+    });
+
+    // THEN
+    expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
+      level: 'warn',
+      message: expect.stringContaining('hotswap'),
+    }));
+  });
+
+  test('does print hotswap warnings for HOTSWAP_ONLY mode', async () => {
+    // WHEN
+    const cx = await builderFixture(toolkit, 'two-empty-stacks');
+    await toolkit.deploy(cx, {
+      hotswap: HotswapMode.HOTSWAP_ONLY,
+    });
+
+    // THEN
+    expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
+      level: 'warn',
+      message: expect.stringContaining('hotswap'),
+    }));
+  });
+});
+
+describe('deploy without hotswap', () => {
+  test('does not print hotswap warnings when mode is undefined', async () => {
+    // WHEN
+    const cx = await builderFixture(toolkit, 'two-empty-stacks');
+    await toolkit.deploy(cx, {
+      hotswap: undefined,
+    });
+
+    // THEN
+    expect(ioHost.notifySpy).not.toHaveBeenCalledWith(expect.objectContaining({
+      level: 'warn',
+      message: expect.stringContaining('hotswap'),
+    }));
+  });
+
+  test('does not print hotswap warning for FULL_DEPLOYMENT mode', async () => {
+    // WHEN
+    const cx = await builderFixture(toolkit, 'two-empty-stacks');
+    await toolkit.deploy(cx, {
+      hotswap: HotswapMode.FULL_DEPLOYMENT,
+    });
+
+    // THEN
+    expect(ioHost.notifySpy).not.toHaveBeenCalledWith(expect.objectContaining({
+      level: 'warn',
+      message: expect.stringContaining('hotswap'),
+    }));
+  });
+});

packages/@aws-cdk/toolkit/test/actions/watch.test.ts

@@ -128,7 +128,11 @@ describe('watch', () => {
     }));
   });
 
-  describe.each([HotswapMode.FALL_BACK, HotswapMode.HOTSWAP_ONLY])('%p mode', (hotswapMode) => {
+  describe.each([
+    [HotswapMode.FALL_BACK, 'on'],
+    [HotswapMode.HOTSWAP_ONLY, 'on'],
+    [HotswapMode.FULL_DEPLOYMENT, 'off'],
+  ])('%p mode', (hotswapMode, userAgent) => {
     test('passes through the correct hotswap mode to deployStack()', async () => {
       // WHEN
       const cx = await builderFixture(toolkit, 'stack-with-role');
@@ -143,10 +147,28 @@ describe('watch', () => {
       // THEN
       expect(deploySpy).toHaveBeenCalledWith(expect.anything(), 'watch', expect.objectContaining({
         hotswap: hotswapMode,
-        extraUserAgent: `cdk-watch/hotswap-${hotswapMode !== HotswapMode.FALL_BACK ? 'on' : 'off'}`,
+        extraUserAgent: `cdk-watch/hotswap-${userAgent}`,
       }));
     });
   });
+
+  test('defaults hotswap to HOTSWAP_ONLY', async () => {
+    // WHEN
+    const cx = await builderFixture(toolkit, 'stack-with-role');
+    ioHost.level = 'warn';
+    await toolkit.watch(cx, {
+      include: [],
+      hotswap: undefined, // force the default
+    });
+
+    await fakeChokidarWatcherOn.readyCallback();
+
+    // THEN
+    expect(deploySpy).toHaveBeenCalledWith(expect.anything(), 'watch', expect.objectContaining({
+      hotswap: HotswapMode.HOTSWAP_ONLY,
+      extraUserAgent: 'cdk-watch/hotswap-on',
+    }));
+  });
 });
 
 // @todo unit test watch with file events