fix(cli): publish assets before deploying any stacks

[misterjoshua]

Changes the CDK CLI to publish assets before deploying any stacks. This allows the CDK CLI to catch docker asset build errors, such as from rate limiting, before any stacks are deployed. Moving publishing this early prevents asset building and publishing failures from interrupting multi-stack deployments part way through.

Fixes #21511

---

All Submissions:

• [x] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [ ] This PR adds new unconventional dependencies following the process described here

New Features

• [ ] Have you added the new feature to an integration test?
  • [ ] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io[bot]]

[misterjoshua]

Here's a demonstration of how this looks on a local test project:

https://user-images.githubusercontent.com/644092/183526428-f16e5488-fa14-41e5-9b3b-7174e1a2fc84.mp4

[misterjoshua]

I'm unsure whether I should be updating a README for this one - should I rename this to a fix so the PR linter doesn't complain?

Edit: I've changed it to a fix for now. We can change it later if needed.

[niebloomj]

Would this update Lambda code also before deploying stacks?

[misterjoshua]

Would this update Lambda code also before deploying stacks?

@niebloomj Nope. Same as before, it puts the assets in a bucket or ECR repository during publishing, but no changes are made to the lambda functions until the CLI deploys the stacks.

Edit: I was re-reading this thread and wondering if I misinterpreted your question the first time. If the question was whether this initial publishing phase works for Lambda .zips as well as Docker images, the answer is yes. Technically, even the synthesized CloudFormation templates are included in this phase. But as with above, changes to the infrastructure don't take effect until after CDK signals to deploy the stacks.

[mrgrain]

@misterjoshua I'm a concerned this PR reverts recent gains made by #19378 Specifically it looks like asset publishing is now a synchronous operation for all stacks.

[misterjoshua]

@misterjoshua I'm a concerned this PR reverts recent gains made by #19378 Specifically it looks like asset publishing is now a synchronous operation for all stacks.

@mrgrain It should be possible to parallelize this build process at the given level of concurrency. I'll see what I can do about that.

[mrgrain]

Does this respect --exclusively ?

[misterjoshua]

Does this respect --exclusively ?

@mrgrain Yes, it does. The stack collection we're drawing from is provided by CdkTooklit.selectStacksForDeploy which handles the --exclusively flag for us. When --exclusively is requested, the stack collection will contain just one stack.

https://github.com/aws/aws-cdk/blob/c81068b34f62a2daef306840cfb44d332d8fe6cf/packages/aws-cdk/lib/cdk-toolkit.ts#L141 https://github.com/aws/aws-cdk/blob/c81068b34f62a2daef306840cfb44d332d8fe6cf/packages/aws-cdk/lib/cdk-toolkit.ts#L630

[misterjoshua]

@misterjoshua I'm a concerned this PR reverts recent gains made by #19378 Specifically it looks like asset publishing is now a synchronous operation for all stacks.

@mrgrain I've added concurrency to the publishing step. Does this alleviate your concern?

[mrgrain]

@mrgrain I've added concurrency to the publishing step. Does this alleviate your concern?

Yes, it does address the speed improvements.

---

In the PR and issue you seem to imply that your motivation of always failing ALL stack deployments when there is an issue with assets in ANY of the stacks is a desirable or even expected behavior. I can see this very much being the case for App with coupled stacks. The current functionality however is that any stacks before the failure will deploy successfully. Admittedly --concurrency is brand-new, but with this flag I believe all other stacks would deploy successfully.

This begs the question to me: Can the change in this PR be the unchangeable default behavior? Should it be an optional flag or even pulled out into separate command?

[rix0rrr]

Hi @misterjoshua, thanks for the good idea. I see how this would definitely help with common pain points.

Unfortunately, this change is going to interfere with future changes I would like to make. Those being:

• treating asset deployments as "just any other type of artifact that can be orchestrated"; as well as
• giving them inputs and outputs.

Doing so would make it possible to do things like: stack 1 creates a bucket, the assets for stack 2 are published into that bucket, then stack 2 gets deployed -- and it all gets cleaned up on deletion.

I'm pretty sure many people would probably much prefer this (it makes it so that the bootstrap stack doesn't need to pre-create S3 buckets and ECR repositories), but that requires that assets are not "special" and just get sequenced like any other deployment.

Treating assets specially and pulling their publishing to the front would not be compatible with that model.

---

In the hypothetical future situation, we can achieve the current behavior in 2 ways:

• Scan the list of assets, build them first, then proceed with deploying as normal; or
• Separate out the building and publishing of assets as 2 different types of actions orchestrated by the CLI, in which case we can push all builds to the front and sequence the deploys as usual.

I think the second solution would have my preference, but I can live with the first.

---

With respect to this PR, in the short term you can try one of these:

• Help work towards our utopia maybe? (😅 -- but it will be a large lift)
• Do the short-circuit solution, in which we pre-build assets (but publish at the normal time)
• Solve the issue locally for yourself, by running cdk-assets before running cdk deploy. Short-circuiting will make sure no duplicate work is done (However, the cdk-assets CLI doesn't have the auth flexibility that the CDK CLI itself has, and depending on your auth setup may or may not work)

[misterjoshua]

... With respect to this PR, in the short term you can try one of these: * Help work towards our utopia maybe? (😅 -- but it will be a large lift) * Do the short-circuit solution, in which we pre-build assets (but publish at the normal time) * Solve the issue locally for yourself, by running cdk-assets before running cdk deploy. Short-circuiting will make sure no duplicate work is done (However, the cdk-assets CLI doesn't have the auth flexibility that the CDK CLI itself has, and depending on your auth setup may or may not work)

@rix0rrr Thanks for these insights. I like your longer-term plan. I'll start looking at the work to do either of your first two options for this PR, as the third about cdk-assets isn't ideal for us.

[misterjoshua]

@mrgrain In light of Rico's recent comments, I'll have to get back to you on your review after I decide with my team which way we're going. Thank you for all your help reviewing so far.

[misterjoshua]

@mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[misterjoshua]

@rix0rrr I've adjusted this PR to separate asset builds from publishing. Is this closer to what you had in mind?

[misterjoshua]

I've fixed a few small oversights and recorded another video to show how this looks for three stacks.

https://user-images.githubusercontent.com/644092/184549129-39fe064d-46cf-48c7-9566-bf914a8151aa.mp4

[mrgrain]

@misterjoshua That looks like the right direction, but let's wait for @rix0rrr 's feedback first.

[misterjoshua]

@misterjoshua That looks like the right direction, but let's wait for @rix0rrr 's feedback first.

Sounds good. I'll keep an eye out for comments. :)

[mergify[bot]]

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]

AWS CodeBuild CI Report

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

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

[mergify[bot]]

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

[rabbittsoup]

Opened issue #21965 due to this behavior change.