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(lambda-python): docker image gets built even when we don't need to bundle assets #16192

Merged
merged 7 commits into from
Mar 4, 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
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-lambda-python/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ export interface BundlingProps extends BundlingOptions {
* @default Architecture.X86_64
*/
readonly architecture?: Architecture;

/**
* Whether or not the bundling process should be skipped
*
* @default - Does not skip bundling
*/
readonly skip?: boolean;
}

/**
Expand All @@ -45,7 +52,7 @@ export class Bundling implements CdkBundlingOptions {
assetHash: options.assetHash,
assetHashType: options.assetHashType,
exclude: DEPENDENCY_EXCLUDES,
bundling: new Bundling(options),
bundling: options.skip ? undefined : new Bundling(options),
});
}

Expand All @@ -72,7 +79,7 @@ export class Bundling implements CdkBundlingOptions {

this.image = image ?? DockerImage.fromBuild(path.join(__dirname, '../lib'), {
buildArgs: {
...props.buildArgs ?? {},
...props.buildArgs,
IMAGE: runtime.bundlingImage.image,
},
platform: architecture.dockerPlatform,
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/lib/function.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as fs from 'fs';
import * as path from 'path';
import { Function, FunctionOptions, Runtime, RuntimeFamily } from '@aws-cdk/aws-lambda';
import { Stack } from '@aws-cdk/core';
import { Bundling } from './bundling';
import { BundlingOptions } from './types';

Expand Down Expand Up @@ -79,6 +80,7 @@ export class PythonFunction extends Function {
code: Bundling.bundle({
entry,
runtime,
skip: !Stack.of(scope).bundlingRequired,
...props.bundling,
}),
handler: resolvedHandler,
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/lib/layer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as path from 'path';
import * as lambda from '@aws-cdk/aws-lambda';
import { Stack } from '@aws-cdk/core';
import { Bundling } from './bundling';
import { BundlingOptions } from './types';

Expand Down Expand Up @@ -67,6 +68,7 @@ export class PythonLayerVersion extends lambda.LayerVersion {
runtime,
architecture,
outputPathSuffix: 'python',
skip: !Stack.of(scope).bundlingRequired,
...props.bundling,
}),
});
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,25 @@ test('Bundling with custom environment vars`', () => {
}),
}));
});

test('Do not build docker image when skipping bundling', () => {
const entry = path.join(__dirname, 'lambda-handler');
Bundling.bundle({
entry: entry,
runtime: Runtime.PYTHON_3_7,
skip: true,
});

expect(DockerImage.fromBuild).not.toHaveBeenCalled();
});

test('Build docker image when bundling is not skipped', () => {
const entry = path.join(__dirname, 'lambda-handler');
Bundling.bundle({
entry: entry,
runtime: Runtime.PYTHON_3_7,
skip: false,
});

expect(DockerImage.fromBuild).toHaveBeenCalled();
});
32 changes: 32 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/test/function.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,35 @@ test('Allows use of custom bundling image', () => {
image,
}));
});

test('Skip bundling when stack does not require it', () => {
const spy = jest.spyOn(stack, 'bundlingRequired', 'get').mockReturnValue(false);
const entry = path.join(__dirname, 'lambda-handler');

new PythonFunction(stack, 'function', {
entry,
runtime: Runtime.PYTHON_3_8,
});

expect(Bundling.bundle).toHaveBeenCalledWith(expect.objectContaining({
skip: true,
}));

spy.mockRestore();
});

test('Do not skip bundling when stack requires it', () => {
const spy = jest.spyOn(stack, 'bundlingRequired', 'get').mockReturnValue(true);
const entry = path.join(__dirname, 'lambda-handler');

new PythonFunction(stack, 'function', {
entry,
runtime: Runtime.PYTHON_3_8,
});

expect(Bundling.bundle).toHaveBeenCalledWith(expect.objectContaining({
skip: false,
}));

spy.mockRestore();
});
30 changes: 30 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/test/layer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,33 @@ test('Allows use of custom bundling image', () => {
image,
}));
});

test('Skip bundling when stack does not require it', () => {
const spy = jest.spyOn(stack, 'bundlingRequired', 'get').mockReturnValue(false);
const entry = path.join(__dirname, 'lambda-handler-project');

new PythonLayerVersion(stack, 'layer', {
entry,
});

expect(Bundling.bundle).toHaveBeenCalledWith(expect.objectContaining({
skip: true,
}));

spy.mockRestore();
});

test('Do not skip bundling when stack requires it', () => {
const spy = jest.spyOn(stack, 'bundlingRequired', 'get').mockReturnValue(true);
const entry = path.join(__dirname, 'lambda-handler-project');

new PythonLayerVersion(stack, 'layer', {
entry,
});

expect(Bundling.bundle).toHaveBeenCalledWith(expect.objectContaining({
skip: false,
}));

spy.mockRestore();
});
5 changes: 1 addition & 4 deletions packages/@aws-cdk/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as path from 'path';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import * as fs from 'fs-extra';
import * as minimatch from 'minimatch';
import { AssetHashType, AssetOptions, FileAssetPackaging } from './assets';
import { BundlingOptions, BundlingOutput } from './bundling';
import { FileSystem, FingerprintOptions } from './fs';
Expand Down Expand Up @@ -188,9 +187,7 @@ export class AssetStaging extends CoreConstruct {
let skip = false;
if (props.bundling) {
// Check if we actually have to bundle for this stack
const bundlingStacks: string[] = this.node.tryGetContext(cxapi.BUNDLING_STACKS) ?? ['*'];
// bundlingStacks is of the form `Stage/Stack`, convert it to `Stage-Stack` before comparing to stack name
skip = !bundlingStacks.find(pattern => minimatch(Stack.of(this).stackName, pattern.replace('/', '-')));
skip = !Stack.of(this).bundlingRequired;
const bundling = props.bundling;
stageThisAsset = () => this.stageByBundling(bundling, skip);
} else {
Expand Down
14 changes: 14 additions & 0 deletions packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { IConstruct, Construct, Node } from 'constructs';
import * as minimatch from 'minimatch';
import { Annotations } from './annotations';
import { App } from './app';
import { Arn, ArnComponents, ArnFormat } from './arn';
Expand Down Expand Up @@ -1153,6 +1154,19 @@ export class Stack extends CoreConstruct implements ITaggable {

return makeStackName(ids);
}

/**
* Indicates whether the stack requires bundling or not
*/
public get bundlingRequired() {
const bundlingStacks: string[] = this.node.tryGetContext(cxapi.BUNDLING_STACKS) ?? ['*'];

// bundlingStacks is of the form `Stage/Stack`, convert it to `Stage-Stack` before comparing to stack name
return bundlingStacks.some(pattern => minimatch(
this.stackName,
pattern.replace('/', '-'),
));
}
}

function merge(template: any, fragment: any): void {
Expand Down
32 changes: 32 additions & 0 deletions packages/@aws-cdk/core/test/stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,38 @@ describe('stack', () => {
expect(new Stack(app, 'Stack', { analyticsReporting: true })._versionReportingEnabled).toBeDefined();

});

test('requires bundling when wildcard is specified in BUNDLING_STACKS', () => {
const app = new App();
const stack = new Stack(app, 'Stack');
stack.node.setContext(cxapi.BUNDLING_STACKS, ['*']);
expect(stack.bundlingRequired).toBe(true);

});

test('requires bundling when stackName has an exact match in BUNDLING_STACKS', () => {
const app = new App();
const stack = new Stack(app, 'Stack');
stack.node.setContext(cxapi.BUNDLING_STACKS, ['Stack']);
expect(stack.bundlingRequired).toBe(true);

});

test('does not require bundling when no item from BUILDING_STACKS matches stackName', () => {
const app = new App();
const stack = new Stack(app, 'Stack');
stack.node.setContext(cxapi.BUNDLING_STACKS, ['Stac']);
expect(stack.bundlingRequired).toBe(false);

});

test('does not require bundling when BUNDLING_STACKS is empty', () => {
const app = new App();
const stack = new Stack(app, 'Stack');
stack.node.setContext(cxapi.BUNDLING_STACKS, []);
expect(stack.bundlingRequired).toBe(false);

});
});

describe('regionalFact', () => {
Expand Down