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

fix(cli): asset prebuild breaks some custom bootstrap scenarios #22930

Merged
merged 2 commits into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions packages/aws-cdk/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@ module.exports = {
branches: 45,
},
},

// We have many tests here that commonly time out
testTimeout: 30_000,
};
50 changes: 41 additions & 9 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,24 @@ export interface CdkToolkitProps {
sdkProvider: SdkProvider;
}

/**
* When to build assets
*/
export enum AssetBuildTime {
/**
* Build all assets before deploying the first stack
*
* This is intended for expensive Docker image builds; so that if the Docker image build
* fails, no stacks are unnecessarily deployed (with the attendant wait time).
*/
ALL_BEFORE_DEPLOY,

/**
* Build assets just-in-time, before publishing
*/
JUST_IN_TIME,
};

/**
* Toolkit logic
*
Expand Down Expand Up @@ -167,17 +185,22 @@ export class CdkToolkit {
}

const stacks = stackCollection.stackArtifacts;
const assetBuildTime = options.assetBuildTime ?? AssetBuildTime.ALL_BEFORE_DEPLOY;


const stackOutputs: { [key: string]: any } = { };
const outputsFile = options.outputsFile;

try {
await buildAllStackAssets(stackCollection.stackArtifacts, {
buildStackAssets: (a) => this.buildAllAssetsForSingleStack(a, options),
});
} catch (e) {
error('\n ❌ Building assets failed: %s', e);
throw e;
if (assetBuildTime === AssetBuildTime.ALL_BEFORE_DEPLOY) {
// Prebuild all assets
try {
await buildAllStackAssets(stackCollection.stackArtifacts, {
buildStackAssets: (a) => this.buildAllAssetsForSingleStack(a, options),
});
} catch (e) {
error('\n ❌ Building assets failed: %s', e);
throw e;
}
}

const deployStack = async (stack: cxapi.CloudFormationStackArtifact) => {
Expand Down Expand Up @@ -257,7 +280,7 @@ export class CdkToolkit {
rollback: options.rollback,
hotswap: options.hotswap,
extraUserAgent: options.extraUserAgent,
buildAssets: false,
buildAssets: assetBuildTime !== AssetBuildTime.ALL_BEFORE_DEPLOY,
assetParallelism: options.assetParallelism,
});

Expand Down Expand Up @@ -1042,6 +1065,15 @@ export interface DeployOptions extends CfnDeployOptions, WatchOptions {
* @default true
*/
readonly assetParallelism?: boolean;

/**
* When to build assets
*
* The default is the Docker-friendly default.
*
* @default AssetBuildTime.ALL_BEFORE_DEPLOY
*/
readonly assetBuildTime?: AssetBuildTime;
}

export interface ImportOptions extends CfnDeployOptions {
Expand Down Expand Up @@ -1136,4 +1168,4 @@ function roundPercentage(num: number): number {
*/
function millisecondsToSeconds(num: number): number {
return num / 1000;
}
}
6 changes: 4 additions & 2 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { execProgram } from '../lib/api/cxapp/exec';
import { PluginHost } from '../lib/api/plugin';
import { ToolkitInfo } from '../lib/api/toolkit-info';
import { StackActivityProgress } from '../lib/api/util/cloudformation/stack-activity-monitor';
import { CdkToolkit } from '../lib/cdk-toolkit';
import { CdkToolkit, AssetBuildTime } from '../lib/cdk-toolkit';
import { realHandler as context } from '../lib/commands/context';
import { realHandler as docs } from '../lib/commands/docs';
import { realHandler as doctor } from '../lib/commands/doctor';
Expand Down Expand Up @@ -157,7 +157,8 @@ async function parseCommandLineArguments() {
"Only in effect if specified alongside the '--watch' option",
})
.option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true })
.option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' }),
.option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' })
.option('asset-prebuild', { type: 'boolean', desc: 'Whether to build all assets before deploying the first stack (useful for failing Docker builds)', default: true }),
)
.command('import [STACK]', 'Import existing resource(s) into the given STACK', (yargs: Argv) => yargs
.option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
Expand Down Expand Up @@ -521,6 +522,7 @@ async function initCommandLine() {
traceLogs: args.logs,
concurrency: args.concurrency,
assetParallelism: configuration.settings.get(['assetParallelism']),
assetBuildTime: configuration.settings.get(['assetPrebuild']) ? AssetBuildTime.ALL_BEFORE_DEPLOY : AssetBuildTime.JUST_IN_TIME,
});

case 'import':
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ export class Settings {
rollback: argv.rollback,
notices: argv.notices,
assetParallelism: argv['asset-parallelism'],
assetPrebuild: argv['asset-prebuild'],
});
}

Expand Down
28 changes: 27 additions & 1 deletion packages/aws-cdk/test/cdk-toolkit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ import { Bootstrapper } from '../lib/api/bootstrap';
import { CloudFormationDeployments, DeployStackOptions, DestroyStackOptions } from '../lib/api/cloudformation-deployments';
import { DeployStackResult } from '../lib/api/deploy-stack';
import { Template } from '../lib/api/util/cloudformation';
import { CdkToolkit, Tag } from '../lib/cdk-toolkit';
import { CdkToolkit, Tag, AssetBuildTime } from '../lib/cdk-toolkit';
import { RequireApproval } from '../lib/diff';
import { flatten } from '../lib/util';
import { instanceMockFrom, MockCloudExecutable, TestStackArtifact, withMocked } from './util';
Expand Down Expand Up @@ -589,6 +589,32 @@ describe('deploy', () => {
}));
});
});

test('can disable asset prebuild', async () => {
// GIVEN
cloudExecutable = new MockCloudExecutable({
stacks: [MockStack.MOCK_STACK_WITH_ASSET],
});
const fakeCloudFormation = new FakeCloudFormation({});

const toolkit = new CdkToolkit({
cloudExecutable,
configuration: cloudExecutable.configuration,
sdkProvider: cloudExecutable.sdkProvider,
cloudFormation: fakeCloudFormation,
});

// WHEN
// Not the best test but following this through to the asset publishing library fails
await withMocked(fakeCloudFormation, 'buildStackAssets', async (mockBuildStackAssets) => {
await toolkit.deploy({
selector: { patterns: ['Test-Stack-Asset'] },
assetBuildTime: AssetBuildTime.JUST_IN_TIME,
});

expect(mockBuildStackAssets).not.toHaveBeenCalled();
});
});
});
});

Expand Down