feat(cli): skip bundling for operations where stack is not needed #9889

jogold · 2020-08-21T11:10:27Z

By default asset bundling is skipped for cdk list and cdk destroy. For cdk deploy, cdk diff
and cdk synthesize the default is to bundle assets for all stacks unless exclusively is specified.
In this case, only the listed stacks will have their assets bundled.

Closes #9540

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

…undling

jogold · 2020-08-27T19:10:35Z

packages/aws-cdk/bin/cdk.ts

@@ -63,10 +63,13 @@ async function parseCommandLineArguments() {
    .option('output', { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true })
    .option('no-color', { type: 'boolean', desc: 'Removes colors and other style from console output', default: false })
    .command(['list [STACKS..]', 'ls [STACKS..]'], 'Lists all stacks in the app', yargs => yargs
-      .option('long', { type: 'boolean', default: false, alias: 'l', desc: 'Display environment information for each stack' }),
+      .option('long', { type: 'boolean', default: false, alias: 'l', desc: 'Display environment information for each stack' })
+      .option('bundling', { type: 'array', default: [], alias: 'b', desc: 'Run bundling only for given stacks (defaults to no stacks)' }),


Do we even need a CLI option for this after all? I don't see why someone would override the defaults here...

yes, seems confusing. my initial idea was to support this as a global feature with a "smart default" so that users will be able to override this behavior if they want.

jogold · 2020-09-02T07:42:08Z

@eladb have you seen this?

eladb · 2020-09-02T08:50:43Z

packages/aws-cdk/bin/cdk.ts

@@ -63,10 +63,13 @@ async function parseCommandLineArguments() {
    .option('output', { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true })
    .option('no-color', { type: 'boolean', desc: 'Removes colors and other style from console output', default: false })
    .command(['list [STACKS..]', 'ls [STACKS..]'], 'Lists all stacks in the app', yargs => yargs
-      .option('long', { type: 'boolean', default: false, alias: 'l', desc: 'Display environment information for each stack' }),
+      .option('long', { type: 'boolean', default: false, alias: 'l', desc: 'Display environment information for each stack' })
+      .option('bundling', { type: 'array', default: [], alias: 'b', desc: 'Run bundling only for given stacks (defaults to no stacks)' }),


yes, seems confusing. my initial idea was to support this as a global feature with a "smart default" so that users will be able to override this behavior if they want.

Pull request has been modified.

rix0rrr

Is this small are of effect even worth it?

How does this work in conjunction with Docker assets? Feels like we wouldn't do staging but we WOULD do build? Isn't that going to break?

rix0rrr · 2020-09-11T08:38:41Z

packages/aws-cdk/lib/settings.ts

+    if (BUNDLING_COMMANDS.includes(argv._[0])) {
+    // If we deploy, diff or synth a list of stacks exclusively we skip
+    // bundling for all other stacks.
+      bundlingStacks = argv.exclusively


This stringly-typed coupling to the yargs configuration in a completely different file is making me very nervous.

Can we at least add these to the definition of Arguments?

If I type the first element of _,

export type Arguments = { readonly [name: string]: unknown, readonly _: [Command, ...string[]], };

Then I'm not sure how I can make the TS compiler happy here?

aws-cdk/packages/aws-cdk/bin/cdk.ts

Line 139 in 0653e6b

const configuration = new Configuration(argv);

Sure, the typing of _ might be tricky.

But for example, exclusively can be added, and TSC knows the type of argv (that it has an exclusively: boolean | undefined field).

Also if you wanted to force it to accept your definition of _ you could do something like this:

const configuration = new Configuration({ ...argv, _: argv._ as [Command, ...string[]] });

(Which is a wacko type, btw. I didn't even know you could do that :) )

jogold · 2020-09-11T09:07:04Z

Is this small are of effect even worth it?

Yes, for apps with lots of bundled assets (Lambda functions) it's really painful to wait for bundling of all assets before seeing the output of cdk ls for example. And when deploying with -e you usually don't want to wait for assets belonging to other stacks to be bundled.

How does this work in conjunction with Docker assets? Feels like we wouldn't do staging but we WOULD do build? Isn't that going to break?

I think it has no impact. Docker assets cannot currently be bundled:

aws-cdk/packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

Line 11 in 54dfe83

export interface DockerImageAssetOptions extends assets.FingerprintOptions {

and so are never staged with bundling specified:

aws-cdk/packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

Lines 131 to 138 in 54dfe83

    
           const staging = new assets.Staging(this, 'Staging', { 
        
             ...props, 
        
             exclude, 
        
             sourcePath: dir, 
        
             extraHash: Object.keys(extraHash).length === 0 
        
               ? undefined 
        
               : JSON.stringify(extraHash), 
        
           });

…undling

mergify · 2020-09-21T08:39:52Z

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

mergify · 2020-09-21T09:07:32Z

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

aws-cdk-automation · 2020-09-21T09:07:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 44b401b
Result: SUCCEEDED
Build Logs (available for 30 days)

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

jogold added 4 commits August 21, 2020 13:09

feat(cli): skip bundling for operations where stack is not needed

2e6f65f

move custom default and test it

6140dab

add tests in core

cb40202

README

f51f73a

jogold marked this pull request as ready for review August 21, 2020 11:59

SomayaB assigned shivlaks Aug 22, 2020

jogold and others added 2 commits August 23, 2020 10:25

tests

86c35c4

Merge branch 'master' into skip-bundling

41c480e

shivlaks requested a review from rix0rrr August 26, 2020 07:10

jogold added 5 commits August 26, 2020 11:00

Merge branch 'master' into skip-bundling

ac2cc2d

revert change in tag-aspect

cebbdb1

correct default

cf2f0e7

Merge branch 'skip-bundling' of github.com:jogold/aws-cdk into skip-b…

f29fe68

…undling

Merge branch 'master' into skip-bundling

b9d3809

jogold commented Aug 27, 2020

View reviewed changes

jogold added 2 commits August 28, 2020 08:52

Merge branch 'master' into skip-bundling

dc923ef

Merge branch 'master' into skip-bundling

88efc5d

eladb previously requested changes Sep 2, 2020

View reviewed changes

jogold added 3 commits September 2, 2020 11:23

Merge branch 'master' into skip-bundling

e00548b

remove CLI option

5e44f79

dummy hash in case of BUNDLE

bd704f4

jogold and others added 3 commits September 2, 2020 11:52

revert changes in bin/cdk.ts

9849358

Merge branch 'master' into skip-bundling

23c5b60

Merge branch 'master' into skip-bundling

8217cff

rix0rrr reviewed Sep 11, 2020

View reviewed changes

Merge branch 'master' into skip-bundling

cb926b4

shivlaks assigned rix0rrr Sep 15, 2020

shivlaks removed their assignment Sep 15, 2020

jogold and others added 4 commits September 16, 2020 14:40

Merge branch 'master' into skip-bundling

266e40b

improve typing

6132e72

Merge branch 'skip-bundling' of github.com:jogold/aws-cdk into skip-b…

749f02b

…undling

Merge branch 'master' into skip-bundling

4e1457f

rix0rrr approved these changes Sep 21, 2020

View reviewed changes

Merge branch 'master' into skip-bundling

44b401b

mergify bot merged commit 28cee39 into aws:master Sep 21, 2020

jogold deleted the skip-bundling branch September 24, 2020 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): skip bundling for operations where stack is not needed #9889

feat(cli): skip bundling for operations where stack is not needed #9889

jogold commented Aug 21, 2020 •

edited

Loading

jogold Aug 27, 2020

eladb Sep 2, 2020

jogold commented Sep 2, 2020

eladb Sep 2, 2020

rix0rrr left a comment

rix0rrr Sep 11, 2020

jogold Sep 11, 2020

rix0rrr Sep 16, 2020

rix0rrr Sep 16, 2020

jogold commented Sep 11, 2020 •

edited

Loading

mergify bot commented Sep 21, 2020

mergify bot commented Sep 21, 2020

aws-cdk-automation commented Sep 21, 2020

feat(cli): skip bundling for operations where stack is not needed #9889

feat(cli): skip bundling for operations where stack is not needed #9889

Conversation

jogold commented Aug 21, 2020 • edited Loading

jogold Aug 27, 2020

Choose a reason for hiding this comment

eladb Sep 2, 2020

Choose a reason for hiding this comment

jogold commented Sep 2, 2020

eladb Sep 2, 2020

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Sep 11, 2020

Choose a reason for hiding this comment

jogold Sep 11, 2020

Choose a reason for hiding this comment

rix0rrr Sep 16, 2020

Choose a reason for hiding this comment

rix0rrr Sep 16, 2020

Choose a reason for hiding this comment

jogold commented Sep 11, 2020 • edited Loading

mergify bot commented Sep 21, 2020

mergify bot commented Sep 21, 2020

aws-cdk-automation commented Sep 21, 2020

AWS CodeBuild CI Report

jogold commented Aug 21, 2020 •

edited

Loading

jogold commented Sep 11, 2020 •

edited

Loading