From 3eccb05c95f318d55006075a826c293eb912518c Mon Sep 17 00:00:00 2001
From: Jonathan Goldwasser <jonathan@goldwasserexchange.be>
Date: Fri, 29 May 2020 14:34:44 +0200
Subject: [PATCH 1/3] fix(core): termination protection not updated when change
 set has no changes

Move termination protection before early return when change set has no changes
---
 packages/aws-cdk/lib/api/deploy-stack.ts      | 22 +++++++++----------
 packages/aws-cdk/test/integ/cli/app/app.js    |  2 +-
 .../aws-cdk/test/integ/cli/cli.integtest.ts   | 12 +++++-----
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
index 99b18c3136b9f..ef5943ce679c6 100644
--- 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 };
 }
 
diff --git a/packages/aws-cdk/test/integ/cli/app/app.js b/packages/aws-cdk/test/integ/cli/app/app.js
index 33e2802ad2ac7..1d0e624ad4479 100644
--- 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.NO_TERMINATION_PROTECTION ? false : true,
 });
 
 app.synth();
diff --git a/packages/aws-cdk/test/integ/cli/cli.integtest.ts b/packages/aws-cdk/test/integ/cli/cli.integtest.ts
index 731f901d90a0d..ba0f6694136fa 100644
--- 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: { NO_TERMINATION_PROTECTION: 'TRUE' } });
+  await cdkDestroy(stackName);
 });
 
 test('cdk synth', async () => {

From 1c1c69dd5783a8a354b6bef680c37040e1cc16f7 Mon Sep 17 00:00:00 2001
From: Jonathan Goldwasser <jonathan@goldwasserexchange.be>
Date: Fri, 29 May 2020 15:00:30 +0200
Subject: [PATCH 2/3] correct boolean comparison

---
 packages/aws-cdk/lib/api/deploy-stack.ts | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts
index ef5943ce679c6..583e6dc3d6ee8 100644
--- a/packages/aws-cdk/lib/api/deploy-stack.ts
+++ b/packages/aws-cdk/lib/api/deploy-stack.ts
@@ -237,7 +237,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
 
   // Update termination protection only if it has changed.
   const terminationProtection = stackArtifact.terminationProtection ?? false;
-  if (cloudFormationStack.terminationProtection !== terminationProtection) {
+  if (!!cloudFormationStack.terminationProtection !== terminationProtection) {
     debug('Updating termination protection from %s to %s for stack %s', cloudFormationStack.terminationProtection, terminationProtection, deployName);
     await cfn.updateTerminationProtection({
       StackName: deployName,
@@ -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;
   }

From 6ce3c14c38e5fa56bad0e1f4099ddf4f63678267 Mon Sep 17 00:00:00 2001
From: Jonathan Goldwasser <jonathan@goldwasserexchange.be>
Date: Tue, 2 Jun 2020 10:29:59 +0200
Subject: [PATCH 3/3] TERMINATION_PROTECTION

---
 packages/aws-cdk/test/integ/cli/app/app.js       | 2 +-
 packages/aws-cdk/test/integ/cli/cli.integtest.ts | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/aws-cdk/test/integ/cli/app/app.js b/packages/aws-cdk/test/integ/cli/app/app.js
index 1d0e624ad4479..84cd0495be95b 100644
--- 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: process.env.NO_TERMINATION_PROTECTION ? false : 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
index ba0f6694136fa..500bd1441d949 100644
--- a/packages/aws-cdk/test/integ/cli/cli.integtest.ts
+++ b/packages/aws-cdk/test/integ/cli/cli.integtest.ts
@@ -46,7 +46,7 @@ test('Termination protection', async () => {
   await expect(cdkDestroy(stackName)).rejects.toThrow('exited with error');
 
   // Can update termination protection even though the change set doesn't contain changes
-  await cdkDeploy(stackName, { modEnv: { NO_TERMINATION_PROTECTION: 'TRUE' } });
+  await cdkDeploy(stackName, { modEnv: { TERMINATION_PROTECTION: 'FALSE' } });
   await cdkDestroy(stackName);
 });