From 1e10bed9d7354cd8fb24492d407d444d2c8636e0 Mon Sep 17 00:00:00 2001 From: ayame113 <40050810+ayame113@users.noreply.github.com> Date: Tue, 7 Jan 2025 00:58:00 +0900 Subject: [PATCH 1/3] fix: commandHook should only be called once in local bundling --- .../aws-lambda-go-alpha/lib/bundling.ts | 6 ++- .../aws-lambda-go-alpha/test/bundling.test.ts | 37 +++++++++++++++ .../aws-lambda-nodejs/lib/bundling.ts | 19 ++++---- .../aws-lambda-nodejs/test/bundling.test.ts | 46 +++++++++++++++++++ 4 files changed, 98 insertions(+), 10 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts b/packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts index 60524d8f20bde..66912aead8255 100644 --- a/packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts +++ b/packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts @@ -152,8 +152,10 @@ export class Bundling implements cdk.BundlingOptions { }) : cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to - const bundlingCommand = this.createBundlingCommand(cdk.AssetStaging.BUNDLING_INPUT_DIR, cdk.AssetStaging.BUNDLING_OUTPUT_DIR); - this.command = props.command ?? ['bash', '-c', bundlingCommand]; + const bundlingCommand = shouldBuildImage + ? this.createBundlingCommand(cdk.AssetStaging.BUNDLING_INPUT_DIR, cdk.AssetStaging.BUNDLING_OUTPUT_DIR) + : undefined; + this.command = props.command ?? (bundlingCommand ? ['bash', '-c', bundlingCommand] : []); this.environment = environment; this.entrypoint = props.entrypoint; this.volumes = props.volumes; diff --git a/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts b/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts index f9a8559add078..ecb15938a7732 100644 --- a/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts +++ b/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts @@ -73,6 +73,7 @@ test('bundling with file as entry', () => { runtime: Runtime.GO_1_X, architecture: Architecture.X86_64, moduleDir, + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { @@ -94,6 +95,7 @@ test('bundling with file in subdirectory as entry', () => { runtime: Runtime.GO_1_X, architecture: Architecture.X86_64, moduleDir, + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { @@ -115,6 +117,7 @@ test('bundling with file other than main.go in subdirectory as entry', () => { runtime: Runtime.GO_1_X, architecture: Architecture.X86_64, moduleDir, + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { @@ -250,6 +253,7 @@ test('Go build flags can be passed', () => { KEY: 'value', }, goBuildFlags: ['-ldflags "-s -w"'], + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { @@ -282,6 +286,7 @@ test('AssetHashType can be specified', () => { KEY: 'value', }, assetHashType: AssetHashType.OUTPUT, + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { @@ -321,6 +326,7 @@ test('with command hooks', () => { return [`cp ${inputDir}/b.txt ${outputDir}/txt`]; }, }, + forcedDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith(path.dirname(moduleDir), { @@ -334,6 +340,37 @@ test('with command hooks', () => { }); }); +test('command hooks should be called once in local bundling', () => { + jest.spyOn(child_process, 'spawnSync').mockReturnValue({ + status: 0, + stderr: Buffer.from('stderr'), + stdout: Buffer.from('stdout'), + pid: 123, + output: ['stdout', 'stderr'], + signal: null, + }); + + const beforeBundling = jest.fn(() => []); + const afterBundling = jest.fn(() => []); + + const bundler = new Bundling({ + entry, + moduleDir, + runtime: Runtime.PROVIDED_AL2023, + architecture: Architecture.X86_64, + commandHooks: { + beforeBundling, + afterBundling, + }, + }); + + const tryBundle = bundler.local?.tryBundle('/outdir', { image: Runtime.GO_1_X.bundlingDockerImage }); + expect(tryBundle).toBe(true); + + expect(beforeBundling.mock.calls).toHaveLength(1); + expect(afterBundling.mock.calls).toHaveLength(1); +}); + test('Custom bundling entrypoint', () => { Bundling.bundle({ entry, diff --git a/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts b/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts index 75333cd9b177b..6cb0a400f0815 100644 --- a/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts +++ b/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts @@ -186,14 +186,17 @@ export class Bundling implements cdk.BundlingOptions { }) : cdk.DockerImage.fromRegistry('dummy'); // Do not build if we don't need to - const bundlingCommand = this.createBundlingCommand({ - inputDir: cdk.AssetStaging.BUNDLING_INPUT_DIR, - outputDir: cdk.AssetStaging.BUNDLING_OUTPUT_DIR, - esbuildRunner: 'esbuild', // esbuild is installed globally in the docker image - tscRunner: 'tsc', // tsc is installed globally in the docker image - osPlatform: 'linux', // linux docker image - }); - this.command = props.command ?? ['bash', '-c', bundlingCommand]; + // create bundling command only if docker bundling is required + const bundlingCommand = shouldBuildImage + ? this.createBundlingCommand({ + inputDir: cdk.AssetStaging.BUNDLING_INPUT_DIR, + outputDir: cdk.AssetStaging.BUNDLING_OUTPUT_DIR, + esbuildRunner: 'esbuild', // esbuild is installed globally in the docker image + tscRunner: 'tsc', // tsc is installed globally in the docker image + osPlatform: 'linux', // linux docker image + }) + : undefined; + this.command = props.command ?? (bundlingCommand ? ['bash', '-c', bundlingCommand] : []); this.environment = props.environment; // Bundling sets the working directory to cdk.AssetStaging.BUNDLING_INPUT_DIR // and we want to force npx to use the globally installed esbuild. diff --git a/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts b/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts index 8470cca534e22..5756ef2d4f9a7 100644 --- a/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts +++ b/packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts @@ -296,6 +296,7 @@ test('esbuild bundling source map default', () => { architecture: Architecture.X86_64, sourceMap: true, sourceMapMode: SourceMapMode.DEFAULT, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -329,6 +330,7 @@ test.each([ depsLockFilePath, runtime: runtime, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -357,6 +359,7 @@ test('esbuild bundling with bundleAwsSdk true with feature flag enabled using No bundleAwsSDK: true, runtime: Runtime.NODEJS_18_X, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -384,6 +387,7 @@ test('esbuild bundling with feature flag enabled using Node Latest', () => { depsLockFilePath, runtime: Runtime.NODEJS_LATEST, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -411,6 +415,7 @@ test('esbuild bundling with feature flag enabled using Node 16', () => { depsLockFilePath, runtime: Runtime.NODEJS_16_X, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -432,6 +437,7 @@ test('esbuild bundling without aws-sdk v3 when use greater than or equal Runtime depsLockFilePath, runtime: Runtime.NODEJS_18_X, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -454,6 +460,7 @@ test('esbuild bundling includes aws-sdk', () => { runtime: Runtime.NODEJS_18_X, architecture: Architecture.X86_64, bundleAwsSDK: true, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -477,6 +484,7 @@ test('esbuild bundling source map inline', () => { architecture: Architecture.X86_64, sourceMap: true, sourceMapMode: SourceMapMode.INLINE, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -502,6 +510,7 @@ test('esbuild bundling is correctly done with custom runtime matching predefined runtime: new Runtime(STANDARD_RUNTIME.name, RuntimeFamily.NODEJS, { supportsInlineCode: true }), architecture: Architecture.X86_64, sourceMapMode: SourceMapMode.INLINE, + forceDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith(path.dirname(depsLockFilePath), { @@ -526,6 +535,7 @@ test('esbuild bundling source map enabled when only source map mode exists', () runtime: STANDARD_RUNTIME, architecture: Architecture.X86_64, sourceMapMode: SourceMapMode.INLINE, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -553,6 +563,7 @@ test('esbuild bundling throws when sourceMapMode used with false sourceMap', () architecture: Architecture.X86_64, sourceMap: false, sourceMapMode: SourceMapMode.INLINE, + forceDockerBundling: true, }); }).toThrow('sourceMapMode cannot be used when sourceMap is false'); }); @@ -761,6 +772,39 @@ test('with command hooks', () => { }); }); +test('command hooks should be called once in local bundling', () => { + jest.spyOn(child_process, 'spawnSync').mockReturnValue({ + status: 0, + stderr: Buffer.from('stderr'), + stdout: Buffer.from('stdout'), + pid: 123, + output: ['stdout', 'stderr'], + signal: null, + }); + + const beforeBundling = jest.fn(() => []); + const afterBundling = jest.fn(() => []); + + const bundler = new Bundling(stack, { + entry, + projectRoot, + depsLockFilePath, + runtime: STANDARD_RUNTIME, + architecture: Architecture.X86_64, + commandHooks: { + beforeBundling, + afterBundling, + beforeInstall() { return []; }, + }, + }); + + const tryBundle = bundler.local?.tryBundle('/outdir', { image: STANDARD_RUNTIME.bundlingDockerImage }); + expect(tryBundle).toBe(true); + + expect(beforeBundling.mock.calls).toHaveLength(1); + expect(afterBundling.mock.calls).toHaveLength(1); +}); + test('esbuild bundling with projectRoot', () => { Bundling.bundle(stack, { entry: '/project/lib/index.ts', @@ -769,6 +813,7 @@ test('esbuild bundling with projectRoot', () => { tsconfig, runtime: STANDARD_RUNTIME, architecture: Architecture.X86_64, + forceDockerBundling: true, }); // Correctly bundles with esbuild @@ -1039,6 +1084,7 @@ test('bundling using NODEJS_LATEST doesn\'t externalize anything by default', () depsLockFilePath, runtime: Runtime.NODEJS_LATEST, architecture: Architecture.X86_64, + forceDockerBundling: true, }); expect(Code.fromAsset).toHaveBeenCalledWith('/project', { From d36dede8fadc03cd49b9ed74bbd2272b026fc374 Mon Sep 17 00:00:00 2001 From: ayame113 <40050810+ayame113@users.noreply.github.com> Date: Sat, 18 Jan 2025 19:27:24 +0900 Subject: [PATCH 2/3] update integ test to assert that beforeBundling/afterBundling are called only once --- .../test/integ.esbuildArgs.ts | 23 +++++++++++++++++++ .../test/integ.function.ts | 20 ++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.esbuildArgs.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.esbuildArgs.ts index 9a2f913547692..2c409548b4d08 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.esbuildArgs.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.esbuildArgs.ts @@ -9,6 +9,10 @@ class TestStack extends Stack { constructor(scope: Construct, id: string, props?: StackProps) { super(scope, id, props); + // assert that beforeBundling/afterBundling are called only once + let beforeBundlingCallCount = 0; + let afterBundlingCallCount = 0; + new lambda.NodejsFunction(this, 'ts-handler', { entry: path.join(__dirname, 'integ-handlers/ts-handler.ts'), runtime: STANDARD_NODEJS_RUNTIME, @@ -20,6 +24,25 @@ class TestStack extends Stack { '--log-limit': '0', '--out-extension': '.js=.mjs', }, + commandHooks: { + beforeBundling() { + if (1 < beforeBundlingCallCount) { + throw new Error('afterBundling should called only once'); + } + beforeBundlingCallCount++; + return []; + }, + afterBundling() { + if (1 < afterBundlingCallCount) { + throw new Error('afterBundling should called only once'); + } + afterBundlingCallCount++; + return []; + }, + beforeInstall() { + return []; + }, + }, }, }); } diff --git a/packages/@aws-cdk/aws-lambda-go-alpha/test/integ.function.ts b/packages/@aws-cdk/aws-lambda-go-alpha/test/integ.function.ts index 9b46387998f09..022a6c3233b9f 100644 --- a/packages/@aws-cdk/aws-lambda-go-alpha/test/integ.function.ts +++ b/packages/@aws-cdk/aws-lambda-go-alpha/test/integ.function.ts @@ -15,11 +15,31 @@ class TestStack extends Stack { constructor(scope: Construct, id: string, props?: StackProps) { super(scope, id, props); + // assert that beforeBundling/afterBundling are called only once + let beforeBundlingCallCount = 0; + let afterBundlingCallCount = 0; + this.lambdaFunction = new lambda.GoFunction(this, 'go-handler-docker', { entry: path.join(__dirname, 'lambda-handler-vendor', 'cmd', 'api'), bundling: { forcedDockerBundling: true, goBuildFlags: ['-mod=readonly', '-ldflags "-s -w"'], + commandHooks: { + beforeBundling() { + if (1 < beforeBundlingCallCount) { + throw new Error('afterBundling should called only once'); + } + beforeBundlingCallCount++; + return []; + }, + afterBundling() { + if (1 < afterBundlingCallCount) { + throw new Error('afterBundling should called only once'); + } + afterBundlingCallCount++; + return []; + }, + }, }, }); } From 781d441333c2fc8b7ea3cdbf6603d2b25f7ce97e Mon Sep 17 00:00:00 2001 From: ayame113 <40050810+ayame113@users.noreply.github.com> Date: Sun, 2 Feb 2025 18:38:06 +0900 Subject: [PATCH 3/3] updating test --- packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts b/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts index 91f2a834a365c..88aea3da16c4e 100644 --- a/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts +++ b/packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts @@ -364,7 +364,7 @@ test('command hooks should be called once in local bundling', () => { }, }); - const tryBundle = bundler.local?.tryBundle('/outdir', { image: Runtime.GO_1_X.bundlingDockerImage }); + const tryBundle = bundler.local?.tryBundle('/outdir', { image: Runtime.GO_1_X.bundlingImage }); expect(tryBundle).toBe(true); expect(beforeBundling.mock.calls).toHaveLength(1);