feat(cli): automatically roll back stacks if necessary #31920

rix0rrr · 2024-10-28T16:26:33Z

If a user is deploying with --no-rollback, and the stack contains replacements (or the --no-rollback flag is dropped), then a rollback needs to be performed before a regular deployment can happen again.

In this PR, we add a prompt where we ask the user to confirm that they are okay with performing a rollback and then a normal deployment.

The way this works is that deployStack detects a disallowed combination (replacement and no-rollback, or being in a stuck state and not being called with no-rollback), and returns a special status code. The driver of the calls, CdkToolkit, will see those special return codes, prompt the user, and retry.

Also get rid of a stray Stack undefined that gets printed to the console.

Closes #30546, Closes #31685

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

If a user is deploying with `--no-rollback`, and the stack contains replacements (or the `--no-rollback` flag is dropped), then a rollback needs to be performed before a regular deployment can happen again. In this PR, we add a prompt where we ask the user to confirm that they are okay with performing a rollback and then a normal deployment. Closes #30546.

rix0rrr · 2024-10-28T16:26:39Z

Still needs tests

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

mrgrain · 2024-10-29T10:58:38Z

If a user is deploying with --no-rollback, and the stack contains replacements (or the --no-rollback flag is dropped),

Is this trying to say that the case applies either when:

the --no-rollback flag is NOT used (i.e. --rollback), or
--no-rollback, and the stack contains replacements

?

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

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

mrgrain · 2024-10-29T11:04:30Z

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

+  ;
+
+export interface RegularDeployStackResult {
+  readonly type: 'did-deploy-stack';


The type system is doing some heavy lifting here. Would a constant prevent having to types this so many times?

The type system is doing some heavy lifting here

Yep! Nice, isn't it?

Would a constant prevent having to types this so many times?

It really doesn't make a difference. This is a string-as-a-type, and therefore just as safe to use as the hypothetical constant const DID_DEPLOY_STACK: 'did-deploy-stack' = 'did-deploy-stack';. The only difference would be 2 keystrokes.

Cool! Does it auto-complete as well?

I'm all for avoiding enums to be honest.

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

mrgrain · 2024-10-29T11:06:13Z

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

+    const confirmed = await promptly.confirm(`${chalk.cyan(question)} (y/n)?`);
+    if (!confirmed) { throw new Error('Aborted by user'); }
+  });
+}


Suggested change

}

}

packages/aws-cdk/lib/diff.ts

mrgrain · 2024-10-29T11:08:11Z

packages/aws-cdk/lib/import.ts

@@ -153,6 +153,10 @@ export class ResourceImporter {
        resourcesToImport,
      });

+      if (result.type !== 'did-deploy-stack') {


This is where const/enum will shine. If there is a typo on the RHS, the check will do unexpected things.

It's already typed. If there's a typo in the RHS, it will not compile.

packages/aws-cdk/lib/import.ts

mrgrain · 2024-10-29T11:12:32Z

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

+      // Do a deployment with a replacement and --force: this will roll back first and then deploy normally
+      phase = '3';
+      await fixture.cdkDeploy('test-rollback', {
+        options: ['--no-rollback', '--force'],


That's a weird UX....

What is weird UX?

--no-rollback --force is a weird DX. Why not just --rollback ?

That will work too. What we are testing is the case where a developer does up + Enter.

The --force is there to skip the interactive prompt, which we have a unit test for but not an integration test.

mrgrain · 2024-11-01T10:41:24Z

packages/aws-cdk/lib/diff.ts

  stackName?: string,
  changeSet?: DescribeChangeSetOutput,
  stream: FormatStream = process.stderr,
 ): boolean {
  const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);

-  // must output the stack name if there are differences, even if quiet
-  if (!quiet || !diff.isEmpty) {
+  if (diffRequiresApproval(diff, requireApproval)) {


Are you sure this isn't changing the behavior of quiet? We are bound to get another issue for that....

What this was doing is always printing Stack undefined when quiet was not set, regardless of whether there was a diff to print. Followed up by the diff if the diff requires approval.

Right now, we only print the stack name if the diff requires approval, period, so it functions as a header.

I suppose the difference would be this:

quiet: false quiet: true Change

Before (no diff) stackName (nothing)

After (no diff) (nothing) (nothing) Now doesn't unnecesessarily print stackName anymore

Before (diff) stackName, diff stackName, diff

After (diff) stackName, diff stackName, diff No change, but confusingly written across 2 statements

The new behavior seems more sensible, no?

Closes #31685

Seems to be what #30186 tried to do but failed-ish.

Proof that the refactoring is safe:

open util/boolean enum Change { PermBroadened, PermNarrowed } enum ConfirmationSetting { Never, Broadened, Any } enum Visible { StackName, Diff } one sig Operation { quiet: Bool, changes: set Change, confirmationSetting: ConfirmationSetting, old: set Visible, new: set Visible, } pred diffRequiresApproval { Operation.confirmationSetting != Never (Operation.confirmationSetting = Broadened and PermBroadened in Operation.changes) or (Operation.confirmationSetting = Any and some Operation.changes) } pred isEmpty[o: Operation] { no o.changes } fact { -- Old (Operation.quiet = False or not Operation.isEmpty) <=> StackName in Operation.old diffRequiresApproval <=> Diff in Operation.old -- New diffRequiresApproval <=> (StackName + Diff) = Operation.new not diffRequiresApproval <=> no Operation.new } check { -- If the new code shows something, the old code also used to show it StackName in Operation.new => StackName in Operation.old Diff in Operation.new => Diff in Operation.old -- This is a restatement of what's above but just to be safe: (Operation.confirmationSetting = Any and some Operation.changes) => Diff in Operation.new (Operation.confirmationSetting = Broadened and PermBroadened in Operation.changes) => Diff in Operation.new }

mrgrain

Conditinally approved that you triple check you didn't accidentally change the behavior of cdk diff --quiet

https://github.com/aws/aws-cdk/pull/31920/files#r1825686382

aws-cdk-automation · 2024-11-04T12:53:36Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-11-05T14:55:38Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-11-05T15:25:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 51b1812
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-11-05T15:25:37Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-11-05T15:25:55Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

rix0rrr requested a review from a team October 28, 2024 16:26

aws-cdk-automation requested a review from a team October 28, 2024 16:26

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Oct 28, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 28, 2024

rix0rrr had a problem deploying to test-pipeline October 28, 2024 16:27 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Oct 28, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Oct 28, 2024

rix0rrr added 2 commits October 29, 2024 09:30

Merge remote-tracking branch 'origin/main' into huijbers/auto-rollback

40acf19

Add integ tests

8a9d85c

rix0rrr had a problem deploying to test-pipeline October 29, 2024 08:55 — with GitHub Actions Failure

Add unit tests

c8c1458

rix0rrr had a problem deploying to test-pipeline October 29, 2024 10:09 — with GitHub Actions Failure

rix0rrr added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Oct 29, 2024

Fix some other tests

f2fb850

rix0rrr had a problem deploying to test-pipeline October 29, 2024 10:30 — with GitHub Actions Failure