Skip to content

Commit

Permalink
fix: commandHook should only be called once in local bundling
Browse files Browse the repository at this point in the history
  • Loading branch information
@ayame113
ayame113 committed Jan 6, 2025
1 parent 3b162fc commit 1e10bed
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 10 deletions.
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts
View file
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
View file
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
View file
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
View file
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

0 comments on commit 1e10bed

Please sign in to comment.