fix(cli): build assets before deploying any stacks #21513

misterjoshua · 2022-08-08T22:28:47Z

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

Fixes #21511

All Submissions:

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 · 2022-08-08T22:28:55Z

misterjoshua · 2022-08-08T22:34:09Z

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

prebuild2.mp4

misterjoshua · 2022-08-08T22:59:04Z

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 · 2022-08-08T23:28:25Z

Would this update Lambda code also before deploying stacks?

misterjoshua · 2022-08-08T23:35:19Z

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.

misterjoshua · 2022-08-09T02:03:00Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+    const shouldPublish = !disableAssetPublishing && !isImporting;
+
+    if (shouldPublish) {
+      await this._publishStackAssets(options.stack, toolkitInfo);
    }


All this is to maintain deployStack()'s backwards compatibility, but I wonder whether publishing assets via deployStack() is even necessary. Given that cdk import is the only other place deployStack() is used and that we're not publishing assets there, I can't think of any good reasons to keep the old behaviour. But, I wanted to discuss this first before making that change.

CloudFormationDeployments is not exposed in v2 (though it was in v1). See #18211

I'd want someone else to confirm this, but from what I can tell it's up for changes.

mrgrain · 2022-08-11T16:47:07Z

@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 · 2022-08-11T16:50:45Z

@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 · 2022-08-11T16:51:04Z

Does this respect --exclusively ?

misterjoshua · 2022-08-11T16:57:46Z

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.

aws-cdk/packages/aws-cdk/lib/cdk-toolkit.ts

Line 141 in c81068b

    
           const stackCollection = await this.selectStacksForDeploy(options.selector, options.exclusively, options.cacheCloudAssembly);

aws-cdk/packages/aws-cdk/lib/cdk-toolkit.ts

Line 630 in c81068b

    
           private async selectStacksForDeploy(selector: StackSelector, exclusively?: boolean, cacheCloudAssembly?: boolean): Promise<StackCollection> {

misterjoshua · 2022-08-11T19:46:04Z

@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 · 2022-08-12T11:00:52Z

@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?

mrgrain

Apologies forgot to post these pending comments.

mrgrain · 2022-08-11T16:34:41Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

@@ -488,3 +510,27 @@ export class CloudFormationDeployments {
    }
  }
 }
+
+/**
+ * Options for publishing stack assets.


Suggested change

* Options for publishing stack assets.

* Options for publishing stack assets

mrgrain · 2022-08-11T16:34:51Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

@@ -451,10 +463,20 @@ export class CloudFormationDeployments {
    };
  }

+  /**
+   * Publish a stack's assets.


Suggested change

* Publish a stack's assets.

* Publish a stack's assets

mrgrain · 2022-08-11T16:38:43Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+  /**
+   * Disable asset publishing
+   *
+   * @default false
+   */
+  readonly disableAssetPublishing?: boolean;


Thinking out loud here, would this be easier to understand if it were this?

Suggested change

/**

* Disable asset publishing

*

* @default false

*/

readonly disableAssetPublishing?: boolean;

/**

* Whether to publish Assets as part of a Stack deployment

*

* @default true

*/

readonly publishAssets?: boolean;

Yes please! Negative arguments break my brains.

mrgrain · 2022-08-11T16:39:26Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

+   * @default 'CDKToolkit'
+   */
+  readonly toolkitStackName?: string;
+}


Suggested change

}

}

mrgrain · 2022-08-11T16:40:56Z

packages/aws-cdk/lib/cdk-toolkit.ts

+        continue;
+      }
+
+      print('🏭 Pre-building and publishing assets for %s\n', stack.displayName);


Suggested change

print('🏭 Pre-building and publishing assets for %s\n', stack.displayName);

print('🏭 Building and publishing assets for %s\n', stack.displayName);

Open to suggestions here, but it feels like it's only "pre" because we know it used to be later in the process. What do you think?

Re-reading this - Building usually occurs at synth time. This might be confusing.

rix0rrr · 2022-08-12T13:27:28Z

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)

rix0rrr

See my previous post

misterjoshua · 2022-08-14T04:12:51Z

@Mergifyio update

mergify · 2022-08-14T04:13:27Z

update

✅ Branch has been successfully updated

misterjoshua · 2022-08-14T06:15:53Z

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

misterjoshua · 2022-08-14T18:08:00Z

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

ice_video_20220814-115842.mp4

mrgrain · 2022-08-16T16:21:42Z

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

misterjoshua · 2022-08-16T16:27:01Z

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

misterjoshua · 2022-08-18T23:30:01Z

packages/aws-cdk/lib/build.ts

+  buildStackAssets: (stack: cxapi.CloudFormationStackArtifact) => Promise<void>;
+};
+
+export async function buildAllStackAssets(stacks: cxapi.CloudFormationStackArtifact[], options: Options): Promise<void> {


This function used to handle --concurrency, but after #21661 reverted --concurrency, we have to build sequentially. As-is, this logic could arguably be moved into CdkToolkit. But, I wanted to wait until I get more feedback on whether this PR is going in the right direction before I make too many changes.

--concurrency is being re-rolled, but I'm honestly not quite sure if building in parallel makes sense. The logs will probably intersperse in unreadable ways if we do that.

mergify · 2022-08-19T11:39:10Z

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 · 2022-08-19T12:35:03Z

AWS CodeBuild CI Report

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

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

mergify · 2022-08-19T12:35:21Z

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

Changes the CDK CLI to build assets before deploying any stacks. This allows the CDK CLI to catch docker build errors, such as from rate limiting, before any stacks are deployed. Moving asset builds this early prevents these build failures from interrupting multi-stack deployments part way through. Fixes aws#21511 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

rabbittsoup · 2022-09-08T13:32:04Z

Opened issue #21965 due to this behavior change.

feat(aws-cdk): add asset prepublishing

eb8f2b4

github-actions bot added feature-request A feature should be added or improved. p2 labels Aug 8, 2022

aws-cdk-automation requested a review from a team August 8, 2022 22:29

misterjoshua changed the title ~~feat(aws-cdk): add asset prepublishing~~ feat(cli): add asset prepublishing Aug 8, 2022

misterjoshua mentioned this pull request Aug 8, 2022

(cli): build+publish all assets used for all selected stacks before deploying any stacks #21511

Closed

2 tasks

misterjoshua changed the title ~~feat(cli): add asset prepublishing~~ feat(cli): publish assets before deploying any stacks Aug 8, 2022

misterjoshua commented Aug 9, 2022

View reviewed changes

add more tests

7da699f

misterjoshua changed the title ~~feat(cli): publish assets before deploying any stacks~~ fix(cli): publish assets before deploying any stacks Aug 10, 2022

misterjoshua and others added 3 commits August 9, 2022 22:46

small fixes

25bdc39

Merge branch 'main' into add-asset-prepublishing

0ce27cb

fix linting

d7cbbee

misterjoshua added 2 commits August 11, 2022 12:12

add publishing concurrency

ba02b1c

internal review changes

5ff973b

misterjoshua requested a review from mrgrain August 11, 2022 19:46

mrgrain previously requested changes Aug 12, 2022

View reviewed changes

rix0rrr previously requested changes Aug 12, 2022

View reviewed changes

misterjoshua added 3 commits August 14, 2022 02:11

clean up

7afd1b6

restore deployStack backward compatibility

2f10ed6

add newline for the old review

d82eb0a

misterjoshua changed the title ~~fix(cli): publish assets before deploying any stacks~~ fix(cli): build assets before deploying any stacks Aug 14, 2022

github-actions bot added the effort/small Small work item – less than a day of effort label Aug 14, 2022

fix instances of poor wording

611b370

Merge branch 'main' into add-asset-prepublishing

f29d4b6

fix small oversights

070d2e6

misterjoshua requested a review from rix0rrr August 15, 2022 00:04

github-actions bot added p1 and removed p2 labels Aug 15, 2022

misterjoshua requested a review from mrgrain August 16, 2022 16:01

misterjoshua and others added 2 commits August 18, 2022 17:06

Merge branch 'main' into add-asset-prepublishing

ba2d57e

fix: error during conflict resolution

fb9bf4d

misterjoshua commented Aug 18, 2022

View reviewed changes

rix0rrr approved these changes Aug 19, 2022

View reviewed changes

Merge branch 'main' into add-asset-prepublishing

9a18370

mergify bot merged commit 5cc0d35 into aws:main Aug 19, 2022

rabbittsoup mentioned this pull request Sep 8, 2022

cli: #21513 breaks custom bootstrap use case #21965

Closed

	* Options for publishing stack assets.
	* Options for publishing stack assets

	print('🏭 Pre-building and publishing assets for %s\n', stack.displayName);
	print('🏭 Building and publishing assets for %s\n', stack.displayName);

fix(cli): build assets before deploying any stacks #21513

fix(cli): build assets before deploying any stacks #21513

Conversation

misterjoshua commented Aug 8, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 8, 2022

misterjoshua commented Aug 8, 2022 • edited Loading

misterjoshua commented Aug 8, 2022 • edited Loading

niebloomj commented Aug 8, 2022

misterjoshua commented Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain commented Aug 11, 2022

misterjoshua commented Aug 11, 2022

mrgrain commented Aug 11, 2022

misterjoshua commented Aug 11, 2022

misterjoshua commented Aug 11, 2022

mrgrain commented Aug 12, 2022

mrgrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Aug 12, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

misterjoshua commented Aug 14, 2022

mergify bot commented Aug 14, 2022

✅ Branch has been successfully updated

misterjoshua commented Aug 14, 2022

misterjoshua commented Aug 14, 2022

mrgrain commented Aug 16, 2022

misterjoshua commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 19, 2022

aws-cdk-automation commented Aug 19, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 19, 2022

rabbittsoup commented Sep 8, 2022

misterjoshua commented Aug 8, 2022 •

edited

Loading

misterjoshua commented Aug 8, 2022 •

edited

Loading

misterjoshua commented Aug 8, 2022 •

edited

Loading

misterjoshua commented Aug 8, 2022 •

edited

Loading