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(cli): --exclusively leads to 0 stacks being selected #21663

Closed
wants to merge 4 commits into from

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Aug 18, 2022

The new concurrency logic introduced in #20345 was checking for all
dependencies to a stack to have been deployed previously for the stack
to be unblocked, but if not all stacks are selected, they will never
become unblocked.

Because there was no check on being "complete" at the end, this silently
leads to no stacks being deployed and the CLI no-opping.

Fix by only waiting for selected stacks among the dependencies, and
adding a safeguard at the end to make sure we deployed all stacks
successfully (or failed halfway through).


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

The new concurrency logic introduced in #20345 was checking for all
dependencies to a stack to have been deployed previously for the stack
to be unblocked, but if not all stacks are selected, they will never
become unblocked.

Because there was no check on being "complete" at the end, this silently
leads to no stacks being deployed and the CLI no-opping.

Fix by only waiting for selected stacks among the dependencies, and
adding a safeguard at the end to make sure we deployed all stacks
successfully (or failed halfway through).
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 18, 2022
@rix0rrr rix0rrr requested a review from a team August 18, 2022 16:27
@rix0rrr rix0rrr self-assigned this Aug 18, 2022
@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2022

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 18, 2022
@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Aug 18, 2022
@github-actions github-actions bot added the p2 label Aug 18, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 18, 2022 16:28
// We shouldn't be able to get here, but check it anyway
const neverUnblocked = Object.entries(deploymentStates).filter(([_, s]) => s === 'pending').map(([n, _]) => n);
if (neverUnblocked.length > 0) {
throw new Error(`The following stacks never became unblocked: ${neverUnblocked.join(', ')}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the Cut us a ticket prompt too?

@relm923
Copy link
Contributor

relm923 commented Aug 18, 2022

@rix0rrr @Naumel happy to take a look at this edge case too. Please let me know if I can help restore the concurrency logic

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Definitely want some tests here.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@rix0rrr rix0rrr closed this Aug 25, 2022
@rix0rrr rix0rrr deleted the huijbers/exclusively-bug branch August 25, 2022 12:38
mergify bot pushed a commit that referenced this pull request Aug 25, 2022
Re-rolls #20345, after it had to be reverted in #21664.

Includes the fix necessary to make sure that `--exclusively` works (#21663), and includes `cdk watch --concurrency` as well (#21598).

Closes #21663.
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
Re-rolls aws#20345, after it had to be reverted in aws#21664.

Includes the fix necessary to make sure that `--exclusively` works (aws#21663), and includes `cdk watch --concurrency` as well (aws#21598).

Closes aws#21663.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants