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): fix so that the commandHook function is called only once during bundling. #32776

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
37 changes: 37 additions & 0 deletions packages/@aws-cdk/aws-lambda-go-alpha/test/bundling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand All @@ -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', {
Expand All @@ -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', {
Expand Down Expand Up @@ -250,6 +253,7 @@ test('Go build flags can be passed', () => {
KEY: 'value',
},
goBuildFlags: ['-ldflags "-s -w"'],
forcedDockerBundling: true,
});

expect(Code.fromAsset).toHaveBeenCalledWith('/project', {
Expand Down Expand Up @@ -282,6 +286,7 @@ test('AssetHashType can be specified', () => {
KEY: 'value',
},
assetHashType: AssetHashType.OUTPUT,
forcedDockerBundling: true,
});

expect(Code.fromAsset).toHaveBeenCalledWith('/project', {
Expand Down Expand Up @@ -321,6 +326,7 @@ test('with command hooks', () => {
return [`cp ${inputDir}/b.txt ${outputDir}/txt`];
},
},
forcedDockerBundling: true,
});

expect(Code.fromAsset).toHaveBeenCalledWith(path.dirname(moduleDir), {
Expand All @@ -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,
Expand Down
19 changes: 11 additions & 8 deletions packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
46 changes: 46 additions & 0 deletions packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -329,6 +330,7 @@ test.each([
depsLockFilePath,
runtime: runtime,
architecture: Architecture.X86_64,
forceDockerBundling: true,
});

// Correctly bundles with esbuild
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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), {
Expand All @@ -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
Expand Down Expand Up @@ -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');
});
Expand Down Expand Up @@ -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',
Expand All @@ -769,6 +813,7 @@ test('esbuild bundling with projectRoot', () => {
tsconfig,
runtime: STANDARD_RUNTIME,
architecture: Architecture.X86_64,
forceDockerBundling: true,
});

// Correctly bundles with esbuild
Expand Down Expand Up @@ -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', {
Expand Down
Loading