feat(cli): skip bundling for operations where stack is not needed#9889
feat(cli): skip bundling for operations where stack is not needed#9889mergify[bot] merged 25 commits intoaws:masterfrom
Conversation
packages/aws-cdk/bin/cdk.ts
Outdated
| .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)' }), |
There was a problem hiding this comment.
Do we even need a CLI option for this after all? I don't see why someone would override the defaults here...
There was a problem hiding this comment.
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.
|
@eladb have you seen this? |
packages/aws-cdk/bin/cdk.ts
Outdated
| .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)' }), |
There was a problem hiding this comment.
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.
rix0rrr
left a comment
There was a problem hiding this comment.
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?
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :) )
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
I think it has no impact. Docker assets cannot currently be bundled: and so are never staged with aws-cdk/packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Lines 131 to 138 in 54dfe83 |
|
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). |
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
By default asset bundling is skipped for
cdk listandcdk destroy. Forcdk deploy,cdk diffand
cdk synthesizethe default is to bundle assets for all stacks unlessexclusivelyis 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