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): allow disabling parallel asset publishing #22579

Merged
merged 2 commits into from Oct 26, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 24 additions & 2 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Expand Up @@ -2,7 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api';
import { AssetManifest } from 'cdk-assets';
import { Tag } from '../cdk-toolkit';
import { debug, warning } from '../logging';
import { buildAssets, publishAssets } from '../util/asset-publishing';
import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions } from '../util/asset-publishing';
import { Mode } from './aws-auth/credentials';
import { ISDK } from './aws-auth/sdk';
import { SdkProvider } from './aws-auth/sdk-provider';
Expand Down Expand Up @@ -253,6 +253,13 @@ export interface DeployStackOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly assetParallelism?: boolean;
}

export interface BuildStackAssetsOptions {
Expand All @@ -274,6 +281,11 @@ export interface BuildStackAssetsOptions {
* @default - Current role
*/
readonly roleArn?: string;

/**
* Options to pass on to `buildAsests()` function
*/
readonly buildOptions?: BuildAssetsOptions;
}

interface PublishStackAssetsOptions {
Expand All @@ -283,6 +295,11 @@ interface PublishStackAssetsOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Options to pass on to `publishAsests()` function
*/
readonly publishOptions?: Omit<PublishAssetsOptions, 'buildAssets'>;
}

export interface DestroyStackOptions {
Expand Down Expand Up @@ -401,6 +418,9 @@ export class CloudFormationDeployments {
if (options.resourcesToImport === undefined) {
await this.publishStackAssets(options.stack, toolkitInfo, {
buildAssets: options.buildAssets ?? true,
publishOptions: {
parallel: options.assetParallelism,
},
});
}

Expand Down Expand Up @@ -434,6 +454,7 @@ export class CloudFormationDeployments {
extraUserAgent: options.extraUserAgent,
resourcesToImport: options.resourcesToImport,
overrideTemplate: options.overrideTemplate,
assetParallelism: options.assetParallelism,
});
}

Expand Down Expand Up @@ -529,7 +550,7 @@ export class CloudFormationDeployments {
toolkitInfo);

const manifest = AssetManifest.fromFile(assetArtifact.file);
await buildAssets(manifest, this.sdkProvider, stackEnv);
await buildAssets(manifest, this.sdkProvider, stackEnv, options.buildOptions);
}
}

Expand All @@ -549,6 +570,7 @@ export class CloudFormationDeployments {

const manifest = AssetManifest.fromFile(assetArtifact.file);
await publishAssets(manifest, this.sdkProvider, stackEnv, {
...options.publishOptions,
buildAssets: options.buildAssets ?? true,
});
}
Expand Down
11 changes: 10 additions & 1 deletion packages/aws-cdk/lib/api/deploy-stack.ts
Expand Up @@ -198,6 +198,13 @@ export interface DeployStackOptions {
* @default - Use the stored template
*/
readonly overrideTemplate?: any;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly assetParallelism?: boolean;
}

export type DeploymentMethod =
Expand Down Expand Up @@ -287,7 +294,9 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
options.toolkitInfo,
options.sdk,
options.overrideTemplate);
await publishAssets(legacyAssets.toManifest(stackArtifact.assembly.directory), options.sdkProvider, stackEnv);
await publishAssets(legacyAssets.toManifest(stackArtifact.assembly.directory), options.sdkProvider, stackEnv, {
parallel: options.assetParallelism,
});

if (options.hotswap) {
// attempt to short-circuit the deployment if possible
Expand Down
15 changes: 14 additions & 1 deletion packages/aws-cdk/lib/cdk-toolkit.ts
Expand Up @@ -258,6 +258,7 @@ export class CdkToolkit {
hotswap: options.hotswap,
extraUserAgent: options.extraUserAgent,
buildAssets: false,
assetParallelism: options.assetParallelism,
});

const message = result.noOp
Expand Down Expand Up @@ -764,7 +765,7 @@ export class CdkToolkit {
}
}

private async buildAllAssetsForSingleStack(stack: cxapi.CloudFormationStackArtifact, options: Pick<DeployOptions, 'roleArn' | 'toolkitStackName'>): Promise<void> {
private async buildAllAssetsForSingleStack(stack: cxapi.CloudFormationStackArtifact, options: Pick<DeployOptions, 'roleArn' | 'toolkitStackName' | 'assetParallelism'>): Promise<void> {
// Check whether the stack has an asset manifest before trying to build and publish.
if (!stack.dependencies.some(cxapi.AssetManifestArtifact.isAssetManifestArtifact)) {
return;
Expand All @@ -775,6 +776,9 @@ export class CdkToolkit {
stack,
roleArn: options.roleArn,
toolkitStackName: options.toolkitStackName,
buildOptions: {
parallel: options.assetParallelism,
},
});
print('\n%s: assets built\n', chalk.bold(stack.displayName));
}
Expand Down Expand Up @@ -1029,6 +1033,15 @@ export interface DeployOptions extends CfnDeployOptions, WatchOptions {
* @default 1
*/
readonly concurrency?: number;

/**
* Build/publish assets for a single stack in parallel
*
* Independent of whether stacks are being done in parallel or no.
*
* @default true
*/
readonly assetParallelism?: boolean;
}

export interface ImportOptions extends CfnDeployOptions {
Expand Down
4 changes: 3 additions & 1 deletion packages/aws-cdk/lib/cli.ts
Expand Up @@ -155,7 +155,8 @@ async function parseCommandLineArguments() {
"'true' by default, use --no-logs to turn off. " +
"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('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' }),

Choose a reason for hiding this comment

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

Is there a reason you did not add the default param for this option?

)
.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 @@ -514,6 +515,7 @@ async function initCommandLine() {
watch: args.watch,
traceLogs: args.logs,
concurrency: args.concurrency,
assetParallelism: args.assetParallelism,
});

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

Expand Down
18 changes: 16 additions & 2 deletions packages/aws-cdk/lib/util/asset-publishing.ts
Expand Up @@ -18,6 +18,13 @@ export interface PublishAssetsOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly parallel?: boolean;
}

/**
Expand All @@ -44,7 +51,7 @@ export async function publishAssets(
aws: new PublishingAws(sdk, targetEnv),
progressListener: new PublishingProgressListener(options.quiet ?? false),
throwOnError: false,
publishInParallel: true,
publishInParallel: options.parallel ?? true,
buildAssets: options.buildAssets ?? true,
publishAssets: true,
});
Expand All @@ -59,6 +66,13 @@ export interface BuildAssetsOptions {
* Print progress at 'debug' level
*/
readonly quiet?: boolean;

/**
* Build assets in parallel
*
* @default true
*/
readonly parallel?: boolean;
}

/**
Expand All @@ -85,7 +99,7 @@ export async function buildAssets(
aws: new PublishingAws(sdk, targetEnv),
progressListener: new PublishingProgressListener(options.quiet ?? false),
throwOnError: false,
publishInParallel: true,
publishInParallel: options.parallel ?? true,
buildAssets: true,
publishAssets: false,
});
Expand Down
Expand Up @@ -904,7 +904,7 @@ test('building assets', async () => {
name: 'aws://account/region',
region: 'region',
});
expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment);
expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment, undefined);
});

function pushStackResourceSummaries(stackName: string, ...items: CloudFormation.StackResourceSummary[]) {
Expand Down
71 changes: 66 additions & 5 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
Expand Up @@ -52,8 +52,11 @@ jest.mock('../lib/logging', () => ({
...jest.requireActual('../lib/logging'),
data: mockData,
}));
jest.setTimeout(30_000);

import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { Manifest } from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { Bootstrapper } from '../lib/api/bootstrap';
import { CloudFormationDeployments, DeployStackOptions, DestroyStackOptions } from '../lib/api/cloudformation-deployments';
Expand All @@ -62,7 +65,8 @@ import { Template } from '../lib/api/util/cloudformation';
import { CdkToolkit, Tag } from '../lib/cdk-toolkit';
import { RequireApproval } from '../lib/diff';
import { flatten } from '../lib/util';
import { instanceMockFrom, MockCloudExecutable, TestStackArtifact } from './util';
import { instanceMockFrom, MockCloudExecutable, TestStackArtifact, withMocked } from './util';
import { MockSdkProvider } from './util/mock-sdk';

let cloudExecutable: MockCloudExecutable;
let bootstrapper: jest.Mocked<Bootstrapper>;
Expand Down Expand Up @@ -555,6 +559,36 @@ describe('deploy', () => {
expect(cloudExecutable.hasApp).toEqual(false);
expect(mockSynthesize).not.toHaveBeenCalled();
});

test('can disable asset parallelism', 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'] },
assetParallelism: false,
});

expect(mockBuildStackAssets).toHaveBeenCalledWith(expect.objectContaining({
buildOptions: expect.objectContaining({
parallel: false,
}),
}));
});
});
});
});

Expand Down Expand Up @@ -911,6 +945,23 @@ class MockStack {
},
displayName: 'Test-Stack-A/witherrors',
}
public static readonly MOCK_STACK_WITH_ASSET: TestStackArtifact = {
stackName: 'Test-Stack-Asset',
template: { Resources: { TemplateName: 'Test-Stack-Asset' } },
env: 'aws://123456789012/bermuda-triangle-1',
assetManifest: {
version: Manifest.version(),
files: {
xyz: {
source: {
path: path.resolve(__dirname, '..', 'LICENSE'),
},
destinations: {
},
},
},
},
}
}

class FakeCloudFormation extends CloudFormationDeployments {
Expand All @@ -921,7 +972,7 @@ class FakeCloudFormation extends CloudFormationDeployments {
expectedTags: { [stackName: string]: { [key: string]: string } } = {},
expectedNotificationArns?: string[],
) {
super({ sdkProvider: undefined as any });
super({ sdkProvider: new MockSdkProvider() });

for (const [stackName, tags] of Object.entries(expectedTags)) {
this.expectedTags[stackName] =
Expand All @@ -934,9 +985,17 @@ class FakeCloudFormation extends CloudFormationDeployments {
}

public deployStack(options: DeployStackOptions): Promise<DeployStackResult> {
expect([MockStack.MOCK_STACK_A.stackName, MockStack.MOCK_STACK_B.stackName, MockStack.MOCK_STACK_C.stackName])
.toContain(options.stack.stackName);
expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]);
expect([
MockStack.MOCK_STACK_A.stackName,
MockStack.MOCK_STACK_B.stackName,
MockStack.MOCK_STACK_C.stackName,
MockStack.MOCK_STACK_WITH_ASSET.stackName,
]).toContain(options.stack.stackName);

if (this.expectedTags[options.stack.stackName]) {
expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]);
}

expect(options.notificationArns).toEqual(this.expectedNotificationArns);
return Promise.resolve({
stackArn: `arn:aws:cloudformation:::stack/${options.stack.stackName}/MockedOut`,
Expand All @@ -959,6 +1018,8 @@ class FakeCloudFormation extends CloudFormationDeployments {
return Promise.resolve({});
case MockStack.MOCK_STACK_C.stackName:
return Promise.resolve({});
case MockStack.MOCK_STACK_WITH_ASSET.stackName:
return Promise.resolve({});
default:
return Promise.reject(`Not an expected mock stack: ${stack.stackName}`);
}
Expand Down