From e32b61652b5d01c44b05c2ac6d5fb1e99b50e059 Mon Sep 17 00:00:00 2001
From: Adam Ruka <adamruka@amazon.com>
Date: Thu, 4 Nov 2021 02:53:56 -0700
Subject: [PATCH] fix(cli): no longer disable rollback by default for hotswap
 deployments (#17317)

Previously, we would default the `--rollback` flag to `false` if it wasn't explicitly provided
if the `--hotswap` option was also passed to `cdk deploy`.
However, disable rollback does not play well with deployments that cause replacements -
they fail, and leave the Stack in a weird state.
For that reason, stop disabling rollback by default.
It's still possible to disable rollback easily if a customer wants to by using the `-R` switch.

Fixes #17267

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
---
 packages/aws-cdk/bin/cdk.ts                    | 9 ++++++---
 packages/aws-cdk/lib/api/deploy-stack.ts       | 2 +-
 packages/aws-cdk/lib/cdk-toolkit.ts            | 1 +
 packages/aws-cdk/test/api/deploy-stack.test.ts | 4 ++--
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/packages/aws-cdk/bin/cdk.ts b/packages/aws-cdk/bin/cdk.ts
index aaaef0deb3182..b5b60f895c8ca 100644
--- a/packages/aws-cdk/bin/cdk.ts
+++ b/packages/aws-cdk/bin/cdk.ts
@@ -105,10 +105,13 @@ async function parseCommandLineArguments() {
       .option('outputs-file', { type: 'string', alias: 'O', desc: 'Path to file where stack outputs will be written as JSON', requiresArg: true })
       .option('previous-parameters', { type: 'boolean', default: true, desc: 'Use previous values for existing parameters (you must specify all parameters on every deployment if this is disabled)' })
       .option('progress', { type: 'string', choices: [StackActivityProgress.BAR, StackActivityProgress.EVENTS], desc: 'Display mode for stack activity events' })
-      .option('rollback', { type: 'boolean', desc: 'Rollback stack to stable state on failure (iterate more rapidly with --no-rollback or -R)' })
+      .option('rollback', {
+        type: 'boolean',
+        desc: "Rollback stack to stable state on failure. Defaults to 'true', iterate more rapidly with --no-rollback or -R. " +
+          'Note: do **not** disable this flag for deployments with resource replacements, as that will always fail',
+      })
       // Hack to get '-R' as an alias for '--no-rollback', suggested by: https://github.com/yargs/yargs/issues/1729
-      .option('R', { type: 'boolean', hidden: true })
-      .middleware(yargsNegativeAlias('R', 'rollback'), true)
+      .option('R', { type: 'boolean', hidden: true }).middleware(yargsNegativeAlias('R', 'rollback'), true)
       .option('hotswap', {
         type: 'boolean',
         desc: "Attempts to perform a 'hotswap' deployment, " +
diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
index ab662d34b517c..2d35a3a01aea0 100644
--- a/packages/aws-cdk/lib/api/deploy-stack.ts
+++ b/packages/aws-cdk/lib/api/deploy-stack.ts
@@ -317,7 +317,7 @@ async function prepareAndExecuteChangeSet(
   if (execute) {
     debug('Initiating execution of changeset %s on stack %s', changeSet.Id, deployName);
 
-    const shouldDisableRollback = (options.rollback === undefined && options.hotswap === true) || options.rollback === false;
+    const shouldDisableRollback = options.rollback === false;
     // Do a bit of contortions to only pass the `DisableRollback` flag if it's true. That way,
     // CloudFormation won't balk at the unrecognized option in regions where the feature is not available yet.
     const disableRollback = shouldDisableRollback ? { DisableRollback: true } : undefined;
diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts
index 65aa443190a57..cd5e591dce866 100644
--- a/packages/aws-cdk/lib/cdk-toolkit.ts
+++ b/packages/aws-cdk/lib/cdk-toolkit.ts
@@ -650,6 +650,7 @@ export interface DeployOptions {
    * @default true
    */
   readonly rollback?: boolean;
+
   /*
    * Whether to perform a 'hotswap' deployment.
    * A 'hotswap' deployment will attempt to short-circuit CloudFormation
diff --git a/packages/aws-cdk/test/api/deploy-stack.test.ts b/packages/aws-cdk/test/api/deploy-stack.test.ts
index 13e4640ff01ec..4770e2ad9528f 100644
--- a/packages/aws-cdk/test/api/deploy-stack.test.ts
+++ b/packages/aws-cdk/test/api/deploy-stack.test.ts
@@ -98,7 +98,7 @@ test("does not call tryHotswapDeployment() if 'hotswap' is false", async () => {
   expect(tryHotswapDeployment).not.toHaveBeenCalled();
 });
 
-test("rollback defaults to disabled if 'hotswap' is true", async () => {
+test("rollback still defaults to enabled even if 'hotswap' is enabled", async () => {
   // WHEN
   await deployStack({
     ...standardDeployStackArguments(),
@@ -107,7 +107,7 @@ test("rollback defaults to disabled if 'hotswap' is true", async () => {
   });
 
   // THEN
-  expect(cfnMocks.executeChangeSet).toHaveBeenCalledWith(expect.objectContaining({
+  expect(cfnMocks.executeChangeSet).not.toHaveBeenCalledWith(expect.objectContaining({
     DisableRollback: true,
   }));
 });