fix(cli): warns about missing --no-rollback flag that is present (#32309)

The logic on `rollback` and `!rollback` was inverted in a couple of places, causing the check to be the wrong way around and making reasoning about these options harder.

Revert the logic and do a more comprehensive test suite around these options.

Also remove a stray `debug()` statement that was left in from a previous PR, and show the stack status in the error message.

Closes #32295.

----

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

Author: rix0rrr

packages/aws-cdk/lib/api/aws-auth/sdk.ts

@@ -951,7 +951,6 @@ export class SDK {
         });
         const command = new GetCallerIdentityCommand({});
         const result = await client.send(command);
-        debug(result.Account!, result.Arn, result.UserId);
         const accountId = result.Account;
         const partition = result.Arn!.split(':')[1];
         if (!accountId) {

packages/aws-cdk/lib/api/deploy-stack.ts

@@ -37,7 +37,7 @@ import { StringWithoutPlaceholders } from './util/placeholders';
 export type DeployStackResult =
   | SuccessfulDeployStackResult
   | NeedRollbackFirstDeployStackResult
-  | ReplacementRequiresNoRollbackStackResult
+  | ReplacementRequiresRollbackStackResult
   ;
 
 /** Successfully deployed a stack */
@@ -52,11 +52,12 @@ export interface SuccessfulDeployStackResult {
 export interface NeedRollbackFirstDeployStackResult {
   readonly type: 'failpaused-need-rollback-first';
   readonly reason: 'not-norollback' | 'replacement';
+  readonly status: string;
 }
 
-/** The upcoming change has a replacement, which requires deploying without --no-rollback */
-export interface ReplacementRequiresNoRollbackStackResult {
-  readonly type: 'replacement-requires-norollback';
+/** The upcoming change has a replacement, which requires deploying with --rollback */
+export interface ReplacementRequiresRollbackStackResult {
+  readonly type: 'replacement-requires-rollback';
 }
 
 export function assertIsSuccessfulDeployStackResult(x: DeployStackResult): asserts x is SuccessfulDeployStackResult {
@@ -512,13 +513,13 @@ class FullCloudFormationDeployment {
     const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable;
     const rollback = this.options.rollback ?? true;
     if (isPausedFailState && replacement) {
-      return { type: 'failpaused-need-rollback-first', reason: 'replacement' };
+      return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name };
     }
-    if (isPausedFailState && !rollback) {
-      return { type: 'failpaused-need-rollback-first', reason: 'not-norollback' };
+    if (isPausedFailState && rollback) {
+      return { type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: this.cloudFormationStack.stackStatus.name };
     }
     if (!rollback && replacement) {
-      return { type: 'replacement-requires-norollback' };
+      return { type: 'replacement-requires-rollback' };
     }
 
     return this.executeChangeSet(changeSetDescription);

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

@@ -436,8 +436,8 @@ export class CdkToolkit {
 
             case 'failpaused-need-rollback-first': {
               const motivation = r.reason === 'replacement'
-                ? 'Stack is in a paused fail state and change includes a replacement which cannot be deployed with "--no-rollback"'
-                : 'Stack is in a paused fail state and command line arguments do not include "--no-rollback"';
+                ? `Stack is in a paused fail state (${r.status}) and change includes a replacement which cannot be deployed with "--no-rollback"`
+                : `Stack is in a paused fail state (${r.status}) and command line arguments do not include "--no-rollback"`;
 
               if (options.force) {
                 warning(`${motivation}. Rolling back first (--force).`);
@@ -461,7 +461,7 @@ export class CdkToolkit {
               break;
             }
 
-            case 'replacement-requires-norollback': {
+            case 'replacement-requires-rollback': {
               const motivation = 'Change includes a replacement which cannot be deployed with "--no-rollback"';
 
               if (options.force) {

packages/aws-cdk/test/api/deploy-stack.test.ts

@@ -1127,30 +1127,41 @@ describe('disable rollback', () => {
 });
 
 test.each([
-  [StackStatus.UPDATE_FAILED, 'failpaused-need-rollback-first'],
-  [StackStatus.CREATE_COMPLETE, 'replacement-requires-norollback'],
-])('no-rollback and replacement is disadvised: %p -> %p', async (stackStatus, expectedType) => {
+  // From a failed state, a --no-rollback is possible as long as there is not a replacement
+  [StackStatus.UPDATE_FAILED, 'no-rollback', 'no-replacement', 'did-deploy-stack'],
+  [StackStatus.UPDATE_FAILED, 'no-rollback', 'replacement', 'failpaused-need-rollback-first'],
+  // Any combination of UPDATE_FAILED & rollback always requires a rollback first
+  [StackStatus.UPDATE_FAILED, 'rollback', 'replacement', 'failpaused-need-rollback-first'],
+  [StackStatus.UPDATE_FAILED, 'rollback', 'no-replacement', 'failpaused-need-rollback-first'],
+  // From a stable state, any deployment containing a replacement requires a regular deployment (--rollback)
+  [StackStatus.UPDATE_COMPLETE, 'no-rollback', 'replacement', 'replacement-requires-rollback'],
+] satisfies Array<[StackStatus, 'rollback' | 'no-rollback', 'replacement' | 'no-replacement', string]>)
+('no-rollback and replacement is disadvised: %s %s %s -> %s', async (stackStatus, rollback, replacement, expectedType) => {
   // GIVEN
   givenTemplateIs(FAKE_STACK.template);
   givenStackExists({
-    NotificationARNs: ['arn:aws:sns:bermuda-triangle-1337:123456789012:TestTopic'],
+    // First call
     StackStatus: stackStatus,
+  }, {
+    // Later calls
+    StackStatus: 'UPDATE_COMPLETE',
   });
-  givenChangeSetContainsReplacement();
+  givenChangeSetContainsReplacement(replacement === 'replacement');
 
   // WHEN
   const result = await deployStack({
     ...standardDeployStackArguments(),
     stack: FAKE_STACK,
-    rollback: false,
+    rollback: rollback === 'rollback',
+    force: true, // Bypass 'canSkipDeploy'
   });
 
   // THEN
   expect(result.type).toEqual(expectedType);
 });
 
 test('assertIsSuccessfulDeployStackResult does what it says', () => {
-  expect(() => assertIsSuccessfulDeployStackResult({ type: 'replacement-requires-norollback' })).toThrow();
+  expect(() => assertIsSuccessfulDeployStackResult({ type: 'replacement-requires-rollback' })).toThrow();
 });
 /**
  * Set up the mocks so that it looks like the stack exists to start with
@@ -1162,12 +1173,14 @@ function givenStackExists(...overrides: Array<Partial<Stack>>) {
     overrides = [{}];
   }
 
+  let handler = mockCloudFormationClient.on(DescribeStacksCommand);
+
   for (const override of overrides.slice(0, overrides.length - 1)) {
-    mockCloudFormationClient.on(DescribeStacksCommand).resolvesOnce({
+    handler = handler.resolvesOnce({
       Stacks: [{ ...baseResponse, ...override }],
     });
   }
-  mockCloudFormationClient.on(DescribeStacksCommand).resolves({
+  handler.resolves({
     Stacks: [{ ...baseResponse, ...overrides[overrides.length - 1] }],
   });
 }
@@ -1178,10 +1191,10 @@ function givenTemplateIs(template: any) {
   });
 }
 
-function givenChangeSetContainsReplacement() {
+function givenChangeSetContainsReplacement(replacement: boolean) {
   mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({
     Status: 'CREATE_COMPLETE',
-    Changes: [
+    Changes: replacement ? [
       {
         Type: 'Resource',
         ResourceChange: {
@@ -1205,6 +1218,6 @@ function givenChangeSetContainsReplacement() {
           ],
         },
       },
-    ],
+    ] : [],
   });
 }

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

@@ -1464,11 +1464,11 @@ describe('synth', () => {
   });
 
   test.each([
-    [{ type: 'failpaused-need-rollback-first', reason: 'replacement' }, false],
-    [{ type: 'failpaused-need-rollback-first', reason: 'replacement' }, true],
-    [{ type: 'failpaused-need-rollback-first', reason: 'not-norollback' }, false],
-    [{ type: 'replacement-requires-norollback' }, false],
-    [{ type: 'replacement-requires-norollback' }, true],
+    [{ type: 'failpaused-need-rollback-first', reason: 'replacement', status: 'OOPS' }, false],
+    [{ type: 'failpaused-need-rollback-first', reason: 'replacement', status: 'OOPS' }, true],
+    [{ type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: 'OOPS' }, false],
+    [{ type: 'replacement-requires-rollback' }, false],
+    [{ type: 'replacement-requires-rollback' }, true],
   ] satisfies Array<[DeployStackResult, boolean]>)('no-rollback deployment that cant proceed will be called with rollback on retry: %p (using force: %p)', async (firstResult, useForce) => {
     cloudExecutable = new MockCloudExecutable({
       stacks: [