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

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

Merged
merged 25 commits into from
Sep 21, 2020
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2e6f65f
feat(cli): skip bundling for operations where stack is not needed
jogold Aug 21, 2020
6140dab
move custom default and test it
jogold Aug 21, 2020
cb40202
add tests in core
jogold Aug 21, 2020
f51f73a
README
jogold Aug 21, 2020
86c35c4
tests
jogold Aug 23, 2020
41c480e
Merge branch 'master' into skip-bundling
jogold Aug 24, 2020
ac2cc2d
Merge branch 'master' into skip-bundling
jogold Aug 26, 2020
cebbdb1
revert change in tag-aspect
jogold Aug 26, 2020
cf2f0e7
correct default
jogold Aug 26, 2020
f29fe68
Merge branch 'skip-bundling' of github.com:jogold/aws-cdk into skip-b…
jogold Aug 26, 2020
b9d3809
Merge branch 'master' into skip-bundling
jogold Aug 27, 2020
dc923ef
Merge branch 'master' into skip-bundling
jogold Aug 28, 2020
88efc5d
Merge branch 'master' into skip-bundling
jogold Sep 2, 2020
e00548b
Merge branch 'master' into skip-bundling
jogold Sep 2, 2020
5e44f79
remove CLI option
jogold Sep 2, 2020
bd704f4
dummy hash in case of BUNDLE
jogold Sep 2, 2020
9849358
revert changes in bin/cdk.ts
jogold Sep 2, 2020
23c5b60
Merge branch 'master' into skip-bundling
jogold Sep 7, 2020
8217cff
Merge branch 'master' into skip-bundling
jogold Sep 8, 2020
cb926b4
Merge branch 'master' into skip-bundling
jogold Sep 11, 2020
266e40b
Merge branch 'master' into skip-bundling
jogold Sep 16, 2020
6132e72
improve typing
jogold Sep 16, 2020
749f02b
Merge branch 'skip-bundling' of github.com:jogold/aws-cdk into skip-b…
jogold Sep 16, 2020
4e1457f
Merge branch 'master' into skip-bundling
jogold Sep 16, 2020
44b401b
Merge branch 'master' into skip-bundling
mergify[bot] Sep 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions packages/@aws-cdk/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { AssetHashType, AssetOptions } from './assets';
import { BundlingOptions } from './bundling';
import { Construct } from './construct-compat';
import { FileSystem, FingerprintOptions } from './fs';
import { Stack } from './stack';
import { Stage } from './stage';

/**
Expand Down Expand Up @@ -97,16 +98,26 @@ export class AssetStaging extends Construct {
const hashType = determineHashType(props.assetHashType, props.assetHash);

if (props.bundling) {
// Determine the source hash in advance of bundling if the asset hash type
// is SOURCE so that the bundler can opt to re-use its previous output.
const sourceHash = hashType === AssetHashType.SOURCE
? this.calculateHash(hashType, props.assetHash, props.bundling)
: undefined;

this.bundleDir = this.bundle(props.bundling, outdir, sourceHash);
this.assetHash = sourceHash ?? this.calculateHash(hashType, props.assetHash, props.bundling);
this.relativePath = renderAssetFilename(this.assetHash);
this.stagedPath = this.relativePath;
// Check if we actually have to bundle for this stack
const bundlingStacks: string[] = this.node.tryGetContext(cxapi.BUNDLING_STACKS) ?? ['*'];
const runBundling = bundlingStacks.includes(Stack.of(this).stackName) || bundlingStacks.includes('*');
if (runBundling) {
// Determine the source hash in advance of bundling if the asset hash type
// is SOURCE so that the bundler can opt to re-use its previous output.
const sourceHash = hashType === AssetHashType.SOURCE
? this.calculateHash(hashType, props.assetHash, props.bundling)
: undefined;

this.bundleDir = this.bundle(props.bundling, outdir, sourceHash);
this.assetHash = sourceHash ?? this.calculateHash(hashType, props.assetHash, props.bundling);
this.relativePath = renderAssetFilename(this.assetHash);
this.stagedPath = this.relativePath;
} else { // Bundling is skipped
this.assetHash = props.assetHashType === AssetHashType.BUNDLE
? this.calculateHash(AssetHashType.CUSTOM, this.node.path) // Use node path as dummy hash because we're not bundling
: this.calculateHash(hashType, props.assetHash);
this.stagedPath = this.sourcePath;
}
} else {
this.assetHash = this.calculateHash(hashType, props.assetHash);

Expand Down
25 changes: 25 additions & 0 deletions packages/@aws-cdk/core/test/test.staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,31 @@ export = {

test.done();
},

'bundling looks at bundling stacks in context'(test: Test) {
// GIVEN
const app = new App();
const stack = new Stack(app, 'MyStack');
stack.node.setContext(cxapi.BUNDLING_STACKS, ['OtherStack']);
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const asset = new AssetStaging(stack, 'Asset', {
sourcePath: directory,
assetHashType: AssetHashType.BUNDLE,
bundling: {
image: BundlingDockerImage.fromRegistry('alpine'),
command: [DockerStubCommand.SUCCESS],
},
});

test.throws(() => readDockerStubInput()); // Bundling did not run
test.equal(asset.assetHash, '3d96e735e26b857743a7c44523c9160c285c2d3ccf273d80fa38a1e674c32cb3'); // hash of MyStack/Asset
test.equal(asset.sourcePath, directory);
test.equal(stack.resolve(asset.stagedPath), directory);

test.done();
},
};

// Reads a docker stub and cleans the volume paths out of the stub.
Expand Down
5 changes: 5 additions & 0 deletions packages/@aws-cdk/cx-api/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,8 @@ export const DISABLE_ASSET_STAGING_CONTEXT = 'aws:cdk:disable-asset-staging';
* Omits stack traces from construct metadata entries.
*/
export const DISABLE_METADATA_STACK_TRACE = 'aws:cdk:disable-stack-trace';

/**
* Run bundling for stacks specified in this context key
*/
export const BUNDLING_STACKS = 'aws:cdk:bundling-stacks';
5 changes: 5 additions & 0 deletions packages/aws-cdk/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ $ cdk doctor
- AWS_SDK_LOAD_CONFIG = 1
```

#### Bundling
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.

### MFA support

If `mfa_serial` is found in the active profile of the shared ini file AWS CDK
Expand Down
7 changes: 5 additions & 2 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { availableInitLanguages, cliInit, printAvailableTemplates } from '../lib
import { data, debug, error, print, setLogLevel } from '../lib/logging';
import { PluginHost } from '../lib/plugin';
import { serializeStructure } from '../lib/serialize';
import { Configuration, Settings } from '../lib/settings';
import { Command, Configuration, Settings } from '../lib/settings';
import * as version from '../lib/version';

/* eslint-disable max-len */
Expand Down Expand Up @@ -136,7 +136,10 @@ async function initCommandLine() {
debug('CDK toolkit version:', version.DISPLAY_VERSION);
debug('Command line arguments:', argv);

const configuration = new Configuration(argv);
const configuration = new Configuration({
...argv,
_: argv._ as [Command, ...string[]], // TypeScript at its best
});
await configuration.load();

const sdkProvider = await SdkProvider.withAwsCliCompatibleDefaults({
Expand Down
3 changes: 3 additions & 0 deletions packages/aws-cdk/lib/api/cxapp/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom
context[cxapi.DISABLE_ASSET_STAGING_CONTEXT] = true;
}

const bundlingStacks = config.settings.get(['bundlingStacks']) ?? ['*'];
context[cxapi.BUNDLING_STACKS] = bundlingStacks;

debug('context:', context);
env[cxapi.CONTEXT_ENV] = JSON.stringify(context);

Expand Down
43 changes: 41 additions & 2 deletions packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,33 @@ export const TRANSIENT_CONTEXT_KEY = '$dontSaveContext';

const CONTEXT_KEY = 'context';

export type Arguments = { readonly [name: string]: unknown };
export enum Command {
LS = 'ls',
LIST = 'list',
DIFF = 'diff',
BOOTSTRAP = 'bootstrap',
DEPLOY = 'deploy',
DESTROY = 'destroy',
SYNTHESIZE = 'synthesize',
SYNTH = 'synth',
METADATA = 'metadata',
INIT = 'init',
VERSION = 'version',
}

const BUNDLING_COMMANDS = [
Command.DEPLOY,
Command.DIFF,
Command.SYNTH,
Command.SYNTHESIZE,
];

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

/**
* All sources of settings combined
Expand Down Expand Up @@ -185,6 +211,18 @@ export class Settings {
const context = this.parseStringContextListToObject(argv);
const tags = this.parseStringTagsListToObject(expectStringList(argv.tags));

// Determine bundling stacks
let bundlingStacks: string[];
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

const configuration = new Configuration(argv);

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

? argv.STACKS ?? ['*']
: ['*'];
} else { // Skip bundling for all stacks
bundlingStacks = [];
}

return new Settings({
app: argv.app,
browser: argv.browser,
Expand All @@ -205,6 +243,7 @@ export class Settings {
staging: argv.staging,
output: argv.output,
progress: argv.progress,
bundlingStacks,
});
}

Expand Down Expand Up @@ -396,4 +435,4 @@ function expectStringList(x: unknown): string[] | undefined {
throw new Error(`Expected list of strings, found ${nonStrings}`);
}
return x;
}
}
2 changes: 1 addition & 1 deletion packages/aws-cdk/test/context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ test('surive no context in old file', async () => {
test('command line context is merged with stored context', async () => {
// GIVEN
await fs.writeJSON('cdk.context.json', { boo: 'far' });
const config = await new Configuration({ context: ['foo=bar'] } as any).load();
const config = await new Configuration({ context: ['foo=bar'], _: ['command'] } as any).load();

// WHEN
expect(config.context.all).toEqual({ foo: 'bar', boo: 'far' });
Expand Down
42 changes: 37 additions & 5 deletions packages/aws-cdk/test/settings.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Context, Settings } from '../lib/settings';
import { Command, Context, Settings } from '../lib/settings';

test('can delete values from Context object', () => {
// GIVEN
Expand Down Expand Up @@ -62,8 +62,8 @@ test('can clear all values in all objects', () => {

test('can parse string context from command line arguments', () => {
// GIVEN
const settings1 = Settings.fromCommandLineArguments({ context: ['foo=bar'] });
const settings2 = Settings.fromCommandLineArguments({ context: ['foo='] });
const settings1 = Settings.fromCommandLineArguments({ context: ['foo=bar'], _: [Command.DEPLOY] });
const settings2 = Settings.fromCommandLineArguments({ context: ['foo='], _: [Command.DEPLOY] });

// THEN
expect(settings1.get(['context']).foo).toEqual( 'bar');
Expand All @@ -72,10 +72,42 @@ test('can parse string context from command line arguments', () => {

test('can parse string context from command line arguments with equals sign in value', () => {
// GIVEN
const settings1 = Settings.fromCommandLineArguments({ context: ['foo==bar='] });
const settings2 = Settings.fromCommandLineArguments({ context: ['foo=bar='] });
const settings1 = Settings.fromCommandLineArguments({ context: ['foo==bar='], _: [Command.DEPLOY] });
const settings2 = Settings.fromCommandLineArguments({ context: ['foo=bar='], _: [Command.DEPLOY] });

// THEN
expect(settings1.get(['context']).foo).toEqual( '=bar=');
expect(settings2.get(['context']).foo).toEqual( 'bar=');
});

test('bundling stacks defaults to an empty list', () => {
// GIVEN
const settings = Settings.fromCommandLineArguments({
_: [Command.LIST],
});

// THEN
expect(settings.get(['bundlingStacks'])).toEqual([]);
});

test('bundling stacks defaults to * for deploy', () => {
// GIVEN
const settings = Settings.fromCommandLineArguments({
_: [Command.DEPLOY],
});

// THEN
expect(settings.get(['bundlingStacks'])).toEqual(['*']);
});

test('bundling stacks with deploy exclusively', () => {
// GIVEN
const settings = Settings.fromCommandLineArguments({
_: [Command.DEPLOY],
exclusively: true,
STACKS: ['cool-stack'],
});

// THEN
expect(settings.get(['bundlingStacks'])).toEqual(['cool-stack']);
});