fix(lambda-python): docker image gets built even when we don't need t…

…o 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 · Mar 4, 2022 · 5dc61ea · 5dc61ea
1 parent fe9f750
commit 5dc61ea
Show file tree

Hide file tree

Showing 9 changed files with 144 additions and 6 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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', () => {