Skip to content

Commit

Permalink
fix(cli): asset prebuild breaks some custom bootstrap scenarios (#22930)
Browse files Browse the repository at this point in the history
If people have managed to adjust the bootstrapping process to have bootstrap resources be created as part of normal stack deploys, the eager asset prebuild breaks the very first deploy (since it requires bootstrap resources to already be present).

Allow disabling the prebuild by passing `--no-asset-prebuild`.

Fixes #21965.


----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Nov 16, 2022
1 parent 741b5e5 commit fc4668d
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 12 deletions.
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

0 comments on commit fc4668d

Please sign in to comment.