From 5dc61eabc0ea3e6294f83db5deb8528461a1d5bc Mon Sep 17 00:00:00 2001 From: jihndai <73680880+jihndai@users.noreply.github.com> Date: Fri, 4 Mar 2022 10:42:51 -0400 Subject: [PATCH] fix(lambda-python): docker image gets built even when we don't need to bundle assets (#16192) `DockerImage.fromBuild` was being called for bundling regardless of whether the stack needed bundling or not. With this change, we will check if the stack needs bundling before proceeding to build the docker image. Fixes #14747 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-lambda-python/lib/bundling.ts | 11 +++++-- .../aws-lambda-python/lib/function.ts | 2 ++ .../@aws-cdk/aws-lambda-python/lib/layer.ts | 2 ++ .../aws-lambda-python/test/bundling.test.ts | 22 +++++++++++++ .../aws-lambda-python/test/function.test.ts | 32 +++++++++++++++++++ .../aws-lambda-python/test/layer.test.ts | 30 +++++++++++++++++ packages/@aws-cdk/core/lib/asset-staging.ts | 5 +-- packages/@aws-cdk/core/lib/stack.ts | 14 ++++++++ packages/@aws-cdk/core/test/stack.test.ts | 32 +++++++++++++++++++ 9 files changed, 144 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda-python/lib/bundling.ts b/packages/@aws-cdk/aws-lambda-python/lib/bundling.ts index 28afa880602fc..14af585f75509 100644 --- a/packages/@aws-cdk/aws-lambda-python/lib/bundling.ts +++ b/packages/@aws-cdk/aws-lambda-python/lib/bundling.ts @@ -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; } /** @@ -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), }); } @@ -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, diff --git a/packages/@aws-cdk/aws-lambda-python/lib/function.ts b/packages/@aws-cdk/aws-lambda-python/lib/function.ts index 7938c20219f73..15013622d2cbc 100644 --- a/packages/@aws-cdk/aws-lambda-python/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda-python/lib/function.ts @@ -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'; @@ -79,6 +80,7 @@ export class PythonFunction extends Function { code: Bundling.bundle({ entry, runtime, + skip: !Stack.of(scope).bundlingRequired, ...props.bundling, }), handler: resolvedHandler, diff --git a/packages/@aws-cdk/aws-lambda-python/lib/layer.ts b/packages/@aws-cdk/aws-lambda-python/lib/layer.ts index d0b9bc6d0f529..516a221b588d8 100644 --- a/packages/@aws-cdk/aws-lambda-python/lib/layer.ts +++ b/packages/@aws-cdk/aws-lambda-python/lib/layer.ts @@ -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'; @@ -67,6 +68,7 @@ export class PythonLayerVersion extends lambda.LayerVersion { runtime, architecture, outputPathSuffix: 'python', + skip: !Stack.of(scope).bundlingRequired, ...props.bundling, }), }); diff --git a/packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts b/packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts index 6bf90aeb090a1..ca2142cce0667 100644 --- a/packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts +++ b/packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts @@ -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(); +}); diff --git a/packages/@aws-cdk/aws-lambda-python/test/function.test.ts b/packages/@aws-cdk/aws-lambda-python/test/function.test.ts index 7eac6ad3df9ec..6fd12540b78c3 100644 --- a/packages/@aws-cdk/aws-lambda-python/test/function.test.ts +++ b/packages/@aws-cdk/aws-lambda-python/test/function.test.ts @@ -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(); +}); diff --git a/packages/@aws-cdk/aws-lambda-python/test/layer.test.ts b/packages/@aws-cdk/aws-lambda-python/test/layer.test.ts index a254e82d48af6..eb12017072030 100644 --- a/packages/@aws-cdk/aws-lambda-python/test/layer.test.ts +++ b/packages/@aws-cdk/aws-lambda-python/test/layer.test.ts @@ -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(); +}); diff --git a/packages/@aws-cdk/core/lib/asset-staging.ts b/packages/@aws-cdk/core/lib/asset-staging.ts index 5ecafd9d3da56..ee1f2bafc6a04 100644 --- a/packages/@aws-cdk/core/lib/asset-staging.ts +++ b/packages/@aws-cdk/core/lib/asset-staging.ts @@ -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'; @@ -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 { diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index 0efd45fc14b3f..d6278d05834e3 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -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'; @@ -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 { diff --git a/packages/@aws-cdk/core/test/stack.test.ts b/packages/@aws-cdk/core/test/stack.test.ts index b344dbbbff0be..e41796bccb624 100644 --- a/packages/@aws-cdk/core/test/stack.test.ts +++ b/packages/@aws-cdk/core/test/stack.test.ts @@ -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', () => {