-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(core): stack termination protection #7610
Conversation
Add a `terminationProtection` prop to `StackProps` to enable stack termination protection. Closes aws#1682
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Looks good. Just need to move the cx-api change to the cloud-assembly-schema
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.
LGTM, @shivlaks please review and approve when ready
@@ -149,13 +149,17 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt | |||
const deployName = options.deployName || stackArtifact.stackName; |
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.
@shivlaks do you mind giving this a quick review?
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.
on it
if (JSON.stringify(stackArtifact.template) === JSON.stringify(await cloudFormationStack.template()) | ||
&& tagsIdentical | ||
&& cloudFormationStack.terminationProtection === terminationProtection) { | ||
debug(`${deployName}: no change in template, tags or termination protection, skipping (use --force to override)`); |
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.
Feels like we should extract all the logic that decides whether a stack should be updated into a separate function now.
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.
done
|
||
// Update termination protection only if it has changed. | ||
if (cloudFormationStack.terminationProtection !== terminationProtection) { | ||
debug('Updating termination protection for stack %s', deployName); |
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.
add the previous and new values of terminationProtection
in this log
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.
done
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Neat! this will be really handy for for users
- It's also probably worth noting somewhere (maybe README) that Nested Stacks inherit this property from their parent stack and that it cannot be set on Nested Stacks.
I also thought about this and initially tried to find a way to prevent this from happening but AFAIU as |
I think it's reasonable since CloudFormation doesn't allow your to set termination directly on nested stacks anyways IIRC. Maybe just mention the behaviour in the README (that termination protection propagates to all nested stacks) |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
feat(core): stack termination protection (#7610)
Add a
terminationProtection
prop toStackProps
to enable stack terminationprotection.
This does not require extra IAM permission for existing CDK stacks
(
cloudformation:UpdateTerminationProtection
).The logic to evaluate if we can skip deploy is now moved to a separate
function.
Closes #1682
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license