-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): automatically roll back stacks if necessary #31920
Changes from 5 commits
1c3db8c
40acf19
8a9d85c
c8c1458
f2fb850
97790eb
87d6ce9
ac22c1c
f35e831
6d00b70
f482b33
417a214
e69e2f5
51b1812
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,17 @@ import { AssetManifestBuilder } from '../util/asset-manifest-builder'; | |
import { determineAllowCrossAccountAssetPublishing } from './util/checks'; | ||
import { publishAssets } from '../util/asset-publishing'; | ||
|
||
export interface DeployStackResult { | ||
export type DeployStackResult = | ||
// Regular result | ||
| RegularDeployStackResult | ||
// The stack is currently in a failpaused state, and needs to be rolled back before the deployment | ||
| { type: 'failpaused-need-rollback-first'; reason: 'not-norollback' | 'replacement' } | ||
// The upcoming change has a replacement, which requires deploying without --no-rollback. | ||
| { type: 'replacement-requires-norollback' } | ||
; | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export interface RegularDeployStackResult { | ||
readonly type: 'did-deploy-stack'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep! Nice, isn't it?
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! Does it auto-complete as well? I'm all for avoiding enums to be honest. |
||
readonly noOp: boolean; | ||
readonly outputs: { [name: string]: string }; | ||
readonly stackArn: string; | ||
|
@@ -279,6 +289,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt | |
print(`\n ${ICON} %s\n`, chalk.bold('hotswap deployment skipped - no changes were detected (use --force to override)')); | ||
} | ||
return { | ||
type: 'did-deploy-stack', | ||
noOp: true, | ||
outputs: cloudFormationStack.outputs, | ||
stackArn: cloudFormationStack.stackId, | ||
|
@@ -326,7 +337,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt | |
print('Falling back to doing a full deployment'); | ||
options.sdk.appendCustomUserAgent('cdk-hotswap/fallback'); | ||
} else { | ||
return { noOp: true, stackArn: cloudFormationStack.stackId, outputs: cloudFormationStack.outputs }; | ||
return { type: 'did-deploy-stack', noOp: true, stackArn: cloudFormationStack.stackId, outputs: cloudFormationStack.outputs }; | ||
} | ||
} | ||
|
||
|
@@ -408,12 +419,26 @@ class FullCloudFormationDeployment { | |
].join('\n')); | ||
} | ||
|
||
return { noOp: true, outputs: this.cloudFormationStack.outputs, stackArn: changeSetDescription.StackId! }; | ||
return { type: 'did-deploy-stack', noOp: true, outputs: this.cloudFormationStack.outputs, stackArn: changeSetDescription.StackId! }; | ||
} | ||
|
||
if (!execute) { | ||
print('Changeset %s created and waiting in review for manual execution (--no-execute)', changeSetDescription.ChangeSetId); | ||
return { noOp: false, outputs: this.cloudFormationStack.outputs, stackArn: changeSetDescription.StackId! }; | ||
return { type: 'did-deploy-stack', noOp: false, outputs: this.cloudFormationStack.outputs, stackArn: changeSetDescription.StackId! }; | ||
} | ||
|
||
// If there are replacements in the changeset, check the rollback flag and stack status | ||
const replacement = hasReplacement(changeSetDescription); | ||
const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; | ||
const rollback = this.options.rollback ?? true; | ||
if (isPausedFailState && replacement) { | ||
return { type: 'failpaused-need-rollback-first', reason: 'replacement' }; | ||
} | ||
if (isPausedFailState && !rollback) { | ||
return { type: 'failpaused-need-rollback-first', reason: 'not-norollback' }; | ||
} | ||
if (!rollback && replacement) { | ||
return { type: 'replacement-requires-norollback' }; | ||
} | ||
|
||
return this.executeChangeSet(changeSetDescription); | ||
|
@@ -439,7 +464,7 @@ class FullCloudFormationDeployment { | |
return waitForChangeSet(this.cfn, this.stackName, changeSetName, { fetchAll: willExecute }); | ||
} | ||
|
||
private async executeChangeSet(changeSet: CloudFormation.DescribeChangeSetOutput): Promise<DeployStackResult> { | ||
private async executeChangeSet(changeSet: CloudFormation.DescribeChangeSetOutput): Promise<RegularDeployStackResult> { | ||
debug('Initiating execution of changeset %s on stack %s', changeSet.ChangeSetId, this.stackName); | ||
|
||
await this.cfn.executeChangeSet({ | ||
|
@@ -478,7 +503,7 @@ class FullCloudFormationDeployment { | |
} | ||
} | ||
|
||
private async directDeployment(): Promise<DeployStackResult> { | ||
private async directDeployment(): Promise<RegularDeployStackResult> { | ||
print('%s: %s stack...', chalk.bold(this.stackName), this.update ? 'updating' : 'creating'); | ||
|
||
const startTime = new Date(); | ||
|
@@ -496,7 +521,7 @@ class FullCloudFormationDeployment { | |
} catch (err: any) { | ||
if (err.message === 'No updates are to be performed.') { | ||
debug('No updates are to be performed for stack %s', this.stackName); | ||
return { noOp: true, outputs: this.cloudFormationStack.outputs, stackArn: this.cloudFormationStack.stackId }; | ||
return { type: 'did-deploy-stack', noOp: true, outputs: this.cloudFormationStack.outputs, stackArn: this.cloudFormationStack.stackId }; | ||
} | ||
throw err; | ||
} | ||
|
@@ -518,7 +543,7 @@ class FullCloudFormationDeployment { | |
} | ||
} | ||
|
||
private async monitorDeployment(startTime: Date, expectedChanges: number | undefined): Promise<DeployStackResult> { | ||
private async monitorDeployment(startTime: Date, expectedChanges: number | undefined): Promise<RegularDeployStackResult> { | ||
const monitor = this.options.quiet ? undefined : StackActivityMonitor.withDefaultPrinter(this.cfn, this.stackName, this.stackArtifact, { | ||
resourcesTotal: expectedChanges, | ||
progress: this.options.progress, | ||
|
@@ -539,7 +564,7 @@ class FullCloudFormationDeployment { | |
await monitor?.stop(); | ||
} | ||
debug('Stack %s has completed updating', this.stackName); | ||
return { noOp: false, outputs: finalState.outputs, stackArn: finalState.stackId }; | ||
return { type: 'did-deploy-stack', noOp: false, outputs: finalState.outputs, stackArn: finalState.stackId }; | ||
} | ||
|
||
/** | ||
|
@@ -718,3 +743,10 @@ function suffixWithErrors(msg: string, errors?: string[]) { | |
function arrayEquals(a: any[], b: any[]): boolean { | ||
return a.every(item => b.includes(item)) && b.every(item => a.includes(item)); | ||
} | ||
|
||
function hasReplacement(cs: AWS.CloudFormation.DescribeChangeSetOutput) { | ||
return (cs.Changes ?? []).some(c => { | ||
const a = c.ResourceChange?.PolicyAction; | ||
return a === 'ReplaceAndDelete' || a === 'ReplaceAndRetain' || a === 'ReplaceAndSnapshot'; | ||
}); | ||
} | ||
rix0rrr marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a weird UX....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is weird UX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-rollback --force
is a weird DX. Why not just--rollback
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.