Skip to content
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

fix(toolkit): endless wait if CDKToolkit stack is REVIEW_IN_PROGRESS #23230

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

RomainMuller
Copy link
Contributor

If a cdk bootstrap operation is interrupted after the ChangeSet has been created, but before it is executed, the CDKToolkit stack will be left in the REVIEW_IN_PROGRESS state, which is a stable state until something or someone either executes or deletes the pending ChangeSet.

This resulted in an endless (and silent, unless cdk bootstrap is executed in verbose mode) wait, as this was considered an "in progress" (aka unstable) state. This changes REVIEW_IN_PROGRESS to be treated as stable, while still debug-logging an indication that we are "ignoring" the existing ChangeSet.

If a `cdk bootstrap` operation is interrupted after the ChangeSet has
been created, but before it is executed, the CDKToolkit stack will be
left in the `REVIEW_IN_PROGRESS` state, which is a stable state until
something or someone either executes or deletes the pending ChangeSet.

This resulted in an endless (and silent, unless `cdk bootstrap` is
executed in verbose mode) wait, as this was considered an "in progress"
(aka unstable) state. This changes `REVIEW_IN_PROGRESS` to be treated as
stable, while still debug-logging an indication that we are "ignoring"
the existing ChangeSet.
@RomainMuller RomainMuller added package/tools Related to AWS CDK Tools or CLI contribution/core This is a PR that came from AWS. pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-breaking-change The PR linter will not require stability in stable modules pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 5, 2022
@RomainMuller RomainMuller requested a review from a team December 5, 2022 11:40
@RomainMuller RomainMuller self-assigned this Dec 5, 2022
@gitpod-io
Copy link

gitpod-io bot commented Dec 5, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team December 5, 2022 11:40
@github-actions github-actions bot added the p2 label Dec 5, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -371,6 +371,14 @@ export async function stabilizeStack(cfn: CloudFormation, stackName: string) {
if (status.isInProgress) {
debug('Stack %s has an ongoing operation in progress and is not stable (%s)', stackName, status);
return undefined;
} else if (status.isReviewInProgress) {
// This may happen if a stack creation operation is interrupted before the ChangeSet execution starts. Recovering
// from this would requiring manual intervention (deleting or executing the pending ChangeSet), abd failing to do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: and

// so will result in an endless wait here (the ChangeSet wont delete or execute itself). Instead of blocking
// "forever" we proceed as if the stack was existing and stable. If there is a concurrent operation that just
// hasn't finished proceeding just yet, either this operation or the concurrent one may fail due to the other one
// having made progress. Which is file. I guess.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fine

Copy link
Contributor

@ryparker ryparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added suggestions for comment typos. G2G either way.

packages/aws-cdk/lib/api/util/cloudformation.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/api/util/cloudformation.ts Outdated Show resolved Hide resolved
@ryparker ryparker added the pr/do-not-merge This PR should not be merged at this time. label Dec 6, 2022
@ryparker
Copy link
Contributor

ryparker commented Dec 6, 2022

Added a hold label so that you can choose to accept the typo fixes or not.

Co-authored-by: Ryan Parker <me@ryanparker.dev>
@RomainMuller RomainMuller added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/do-not-merge This PR should not be merged at this time. labels Dec 7, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 477fa85 into main Dec 7, 2022
@mergify mergify bot deleted the rmuller/fix-toolkit-review-in-progress branch December 7, 2022 15:01
@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

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).

brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Dec 9, 2022
aws#23230)

If a `cdk bootstrap` operation is interrupted after the ChangeSet has been created, but before it is executed, the CDKToolkit stack will be left in the `REVIEW_IN_PROGRESS` state, which is a stable state until something or someone either executes or deletes the pending ChangeSet.

This resulted in an endless (and silent, unless `cdk bootstrap` is executed in verbose mode) wait, as this was considered an "in progress" (aka unstable) state. This changes `REVIEW_IN_PROGRESS` to be treated as stable, while still debug-logging an indication that we are "ignoring" the existing ChangeSet.
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
aws#23230)

If a `cdk bootstrap` operation is interrupted after the ChangeSet has been created, but before it is executed, the CDKToolkit stack will be left in the `REVIEW_IN_PROGRESS` state, which is a stable state until something or someone either executes or deletes the pending ChangeSet.

This resulted in an endless (and silent, unless `cdk bootstrap` is executed in verbose mode) wait, as this was considered an "in progress" (aka unstable) state. This changes `REVIEW_IN_PROGRESS` to be treated as stable, while still debug-logging an indication that we are "ignoring" the existing ChangeSet.
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
aws#23230)

If a `cdk bootstrap` operation is interrupted after the ChangeSet has been created, but before it is executed, the CDKToolkit stack will be left in the `REVIEW_IN_PROGRESS` state, which is a stable state until something or someone either executes or deletes the pending ChangeSet.

This resulted in an endless (and silent, unless `cdk bootstrap` is executed in verbose mode) wait, as this was considered an "in progress" (aka unstable) state. This changes `REVIEW_IN_PROGRESS` to be treated as stable, while still debug-logging an indication that we are "ignoring" the existing ChangeSet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 package/tools Related to AWS CDK Tools or CLI pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-breaking-change The PR linter will not require stability in stable modules pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants