fix(cli): termination protection not updated when change set has no c…

…hanges (#8275) Move termination protection **before** early return when change set has no changes Also fixes the fact that `updateTermination` was called when it was not necessary. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Jun 3, 2020 · 29d3145 · 29d3145
1 parent dfdcd90
commit 29d3145
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 20 deletions.
diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
@@ -235,6 +235,17 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
   debug('Initiated creation of changeset: %s; waiting for it to finish creating...', changeSet.Id);
   const changeSetDescription = await waitForChangeSet(cfn, deployName, changeSetName);
 
+  // Update termination protection only if it has changed.
+  const terminationProtection = stackArtifact.terminationProtection ?? false;
+  if (!!cloudFormationStack.terminationProtection !== terminationProtection) {
+    debug('Updating termination protection from %s to %s for stack %s', cloudFormationStack.terminationProtection, terminationProtection, deployName);
+    await cfn.updateTerminationProtection({
+      StackName: deployName,
+      EnableTerminationProtection: terminationProtection,
+    }).promise();
+    debug('Termination protection updated to %s for stack %s', terminationProtection, deployName);
+  }
+
   if (changeSetHasNoChanges(changeSetDescription)) {
     debug('No changes are to be performed on %s.', deployName);
     await cfn.deleteChangeSet({ StackName: deployName, ChangeSetName: changeSetName }).promise();
@@ -262,17 +273,6 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
     print('Changeset %s created and waiting in review for manual execution (--no-execute)', changeSetName);
   }
 
-  // Update termination protection only if it has changed.
-  const terminationProtection = stackArtifact.terminationProtection ?? false;
-  if (cloudFormationStack.terminationProtection !== terminationProtection) {
-    debug('Updating termination protection from %s to %s for stack %s', cloudFormationStack.terminationProtection, terminationProtection, deployName);
-    await cfn.updateTerminationProtection({
-      StackName: deployName,
-      EnableTerminationProtection: terminationProtection,
-    }).promise();
-    debug('Termination protection updated to %s for stack %s', terminationProtection, deployName);
-  }
-
   return { noOp: false, outputs: cloudFormationStack.outputs, stackArn: changeSet.StackId!, stackArtifact };
 }
 
@@ -399,8 +399,7 @@ async function canSkipDeploy(deployStackOptions: DeployStackOptions, cloudFormat
   }
 
   // Termination protection has been updated
-  const terminationProtection = deployStackOptions.stack.terminationProtection ?? false; // cast to boolean for comparison
-  if (terminationProtection !== cloudFormationStack.terminationProtection) {
+  if (!!deployStackOptions.stack.terminationProtection !== !!cloudFormationStack.terminationProtection) {
     debug(`${deployName}: termination protection has been updated`);
     return false;
   }

diff --git a/packages/aws-cdk/test/integ/cli/app/app.js b/packages/aws-cdk/test/integ/cli/app/app.js
@@ -280,7 +280,7 @@ new StackWithNestedStack(app, `${stackPrefix}-with-nested-stack`);
 new StackWithNestedStackUsingParameters(app, `${stackPrefix}-with-nested-stack-using-parameters`);
 
 new YourStack(app, `${stackPrefix}-termination-protection`, {
-  terminationProtection: true,
+  terminationProtection: process.env.TERMINATION_PROTECTION !== 'FALSE' ? true : false,
 });
 
 app.synth();
diff --git a/packages/aws-cdk/test/integ/cli/cli.integtest.ts b/packages/aws-cdk/test/integ/cli/cli.integtest.ts
@@ -39,15 +39,15 @@ test('Two ways of shoing the version', async () => {
 });
 
 test('Termination protection', async () => {
-  await cdkDeploy('termination-protection');
+  const stackName = 'termination-protection';
+  await cdkDeploy(stackName);
 
   // Try a destroy that should fail
-  await expect(cdkDestroy('termination-protection')).rejects.toThrow('exited with error');
+  await expect(cdkDestroy(stackName)).rejects.toThrow('exited with error');
 
-  await cloudFormation('updateTerminationProtection', {
-    EnableTerminationProtection: false,
-    StackName: fullStackName('termination-protection'),
-  });
+  // Can update termination protection even though the change set doesn't contain changes
+  await cdkDeploy(stackName, { modEnv: { TERMINATION_PROTECTION: 'FALSE' } });
+  await cdkDestroy(stackName);
 });
 
 test('cdk synth', async () => {