feat(cli): automatically roll back stacks if necessary#31920
feat(cli): automatically roll back stacks if necessary#31920mergify[bot] merged 14 commits intomainfrom
Conversation
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.
|
Still needs tests |
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
Is this trying to say that the case applies either when:
? |
| ; | ||
|
|
||
| export interface RegularDeployStackResult { | ||
| readonly type: 'did-deploy-stack'; |
There was a problem hiding this comment.
The type system is doing some heavy lifting here. Would a constant prevent having to types this so many times?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool! Does it auto-complete as well?
I'm all for avoiding enums to be honest.
packages/aws-cdk/lib/cdk-toolkit.ts
Outdated
| const confirmed = await promptly.confirm(`${chalk.cyan(question)} (y/n)?`); | ||
| if (!confirmed) { throw new Error('Aborted by user'); } | ||
| }); | ||
| } No newline at end of file |
packages/aws-cdk/lib/import.ts
Outdated
| resourcesToImport, | ||
| }); | ||
|
|
||
| if (result.type !== 'did-deploy-stack') { |
There was a problem hiding this comment.
This is where const/enum will shine. If there is a typo on the RHS, the check will do unexpected things.
There was a problem hiding this comment.
It's already typed. If there's a typo in the RHS, it will not compile.
| // 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'], |
There was a problem hiding this comment.
--no-rollback --force is a weird DX. Why not just --rollback ?
There was a problem hiding this comment.
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.
|
|
||
| // must output the stack name if there are differences, even if quiet | ||
| if (!quiet || !diff.isEmpty) { | ||
| if (diffRequiresApproval(diff, requireApproval)) { |
There was a problem hiding this comment.
Are you sure this isn't changing the behavior of quiet? We are bound to get another issue for that....
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Conditinally approved that you triple check you didn't accidentally change the behavior of cdk diff --quiet
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
If a user is deploying with
--no-rollback, and the stack contains replacements (or the--no-rollbackflag 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
deployStackdetects 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 undefinedthat 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