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

feat(lambda-nodejs): use docker instead of npm for parcel-bundler #7169

Merged
merged 32 commits into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ea0e032
feat(lambda-nodejs): use docker instead of npm package for parcel-bun…
AlexCheema Apr 3, 2020
3f9db92
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 3, 2020
4490601
require version ^1 of parcel-bundler in Dockerfile
AlexCheema Apr 4, 2020
157281e
update README: docker requirement
AlexCheema Apr 4, 2020
74383e2
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 4, 2020
e744f9c
comment in Dockerfile, make node version a build ARG
AlexCheema Apr 6, 2020
e1428ae
add test for when docker not installed
AlexCheema Apr 6, 2020
9a5cb5c
add optional nodeDockerTag to props
AlexCheema Apr 6, 2020
370778d
Merge branch 'master' into alexcheema/docker-parcel-bundler
AlexCheema Apr 6, 2020
d36a005
Expose nodeDockerTag in NodejsFunctionProps and pass it to the Builder
AlexCheema Apr 6, 2020
23ffd92
Merge branch 'alexcheema/docker-parcel-bundler' of https://github.com…
AlexCheema Apr 6, 2020
459e587
cdk-build pre
AlexCheema Apr 6, 2020
b64686d
Merge branch 'master' into alexcheema/docker-parcel-bundler
AlexCheema Apr 6, 2020
5fc915d
Merge branch 'master' into alexcheema/docker-parcel-bundler
AlexCheema Apr 7, 2020
9842450
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 14, 2020
34afc14
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 15, 2020
6f8fd3c
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 15, 2020
0404baa
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 22, 2020
ebb464f
chore(lambda-nodejs): remove dockerd pre directive
AlexCheema Apr 24, 2020
5f9d9ab
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 24, 2020
eaf48b2
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 26, 2020
86ee7d2
add missing closing parenthesis
Apr 27, 2020
2f63cab
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 29, 2020
a78b7e0
fix linter errors
AlexCheema Apr 29, 2020
24d9e8d
fix linter error
AlexCheema Apr 29, 2020
5fa1e63
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema Apr 29, 2020
c7852e9
start docker daemon inside the container
Apr 30, 2020
9f85d87
Update buildspec.yaml
Apr 30, 2020
d3c79d4
Fix docker location.
Apr 30, 2020
feadb67
Update buildspec.yaml
Apr 30, 2020
711b58e
Merge remote-tracking branch 'origin/master' into alexcheema/docker-p…
AlexCheema May 1, 2020
8de8246
Merge branch 'master' into alexcheema/docker-parcel-bundler
mergify[bot] May 1, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 1 addition & 8 deletions packages/@aws-cdk/aws-lambda-nodejs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,7 @@

This library provides constructs for Node.js Lambda functions.

To use this module, you will need to add a dependency on `parcel-bundler` in your
AlexCheema marked this conversation as resolved.
Show resolved Hide resolved
`package.json`:

```
yarn add parcel-bundler@^1
# or
npm install parcel-bundler@^1
```
To use this module, you will need to have Docker installed.

### Node.js Function
Define a `NodejsFunction`:
Expand Down
48 changes: 23 additions & 25 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,7 @@ export interface BuilderOptions {
* Builder
*/
export class Builder {
private readonly parcelBinPath: string;

constructor(private readonly options: BuilderOptions) {
let parcelPkgPath: string;
try {
parcelPkgPath = require.resolve('parcel-bundler/package.json'); // This will throw if `parcel-bundler` cannot be found
} catch (err) {
throw new Error('It looks like parcel-bundler is not installed. Please install v1.x of parcel-bundler with yarn or npm.');
}
const parcelDir = path.dirname(parcelPkgPath);
const parcelPkg = JSON.parse(fs.readFileSync(parcelPkgPath, 'utf8'));

if (!parcelPkg.version || !/^1\./.test(parcelPkg.version)) { // Peer dependency on parcel v1.x
throw new Error(`This module has a peer dependency on parcel-bundler v1.x. Got v${parcelPkg.version}.`);
}

this.parcelBinPath = path.join(parcelDir, parcelPkg.bin.parcel);
}

public build(): void {
Expand All @@ -78,29 +62,43 @@ export class Builder {
});
}

const args = [
'build', this.options.entry,
'--out-dir', this.options.outDir,
const dockerBuildArgs = [
"build", "-t", "parcel-bundler", path.join(__dirname, "../parcel-bundler")
];
const dockerRunArgs = [
"run", "--rm",
"-v", `${path.dirname(path.resolve(this.options.entry))}:/entry`,
"-v", `${path.resolve(this.options.outDir)}:/out`,
...(this.options.cacheDir ? ["-v", `${path.resolve(this.options.cacheDir)}:/cache`] : []),
"parcel-bundler"
];
const parcelArgs = [
'parcel', 'build', `/entry/${path.basename(this.options.entry)}`,
'--out-dir', "/out",
'--out-file', 'index.js',
'--global', this.options.global,
'--target', 'node',
'--bundle-node-modules',
'--log-level', '2',
!this.options.minify && '--no-minify',
!this.options.sourceMaps && '--no-source-maps',
...this.options.cacheDir
? ['--cache-dir', this.options.cacheDir]
: [],
...(this.options.cacheDir ? ['--cache-dir', "/cache"] : []),
].filter(Boolean) as string[];

const parcel = spawnSync(this.parcelBinPath, args);
const build = spawnSync("docker", dockerBuildArgs);
if (build.error) {
throw build.error;
}
if (build.status !== 0) {
throw new Error(`[Status ${build.status}] stdout: ${build.stdout?.toString().trim()}\n\n\nstderr: ${build.stderr?.toString().trim()}`);
}

const parcel = spawnSync("docker", [...dockerRunArgs, ...parcelArgs]);
if (parcel.error) {
throw parcel.error;
}

if (parcel.status !== 0) {
throw new Error(parcel.stdout.toString().trim());
throw new Error(`[Status ${parcel.status}] stdout: ${parcel.stdout?.toString().trim()}\n\n\nstderr: ${parcel.stderr?.toString().trim()}`);
}
} catch (err) {
throw new Error(`Failed to build file at ${this.options.entry}: ${err}`);
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@
"cdk-build-tools": "0.0.0",
"cdk-integ-tools": "0.0.0",
"fs-extra": "^8.1.0",
"parcel-bundler": "^1.12.4",
"pkglint": "0.0.0"
},
"dependencies": {
Expand Down
5 changes: 5 additions & 0 deletions packages/@aws-cdk/aws-lambda-nodejs/parcel-bundler/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM node:13.8.0-alpine3.11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a wee comment explaining what this docker image is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FROM node:13.8.0-alpine3.11
ARG VERSION=13.8.0-alpine3.11
FROM node:$VERSION

With a prop allowing to customize the version? and the default either in the Dockerfile as above or in the .ts file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expose this build arg as an optional prop in NodejsFunctionProps and then BuilderOptions? There's a risk of being stucked with a parcel-bundler version that could be incompatible with the fixed node/alpine version of the Dockerfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expose nodeDockerTag in NodejsFunctionProps and pass it to the Builder and we're done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


RUN yarn global add parcel-bundler@^1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not a prebuilt Parcel image up on DockerHub that we can use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really... also the idea is to maybe customize this image to address this #6323 and support externals (like aws-sdk).


CMD [ "parcel" ]
52 changes: 20 additions & 32 deletions packages/@aws-cdk/aws-lambda-nodejs/test/builder.test.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,22 @@
import { spawnSync } from 'child_process';
import * as fs from 'fs';
import * as path from 'path';
import { Builder } from '../lib/builder';

let parcelPkgPath: string;
let parcelPkg: Buffer;
beforeAll(() => {
parcelPkgPath = require.resolve('parcel-bundler/package.json');
parcelPkg = fs.readFileSync(parcelPkgPath);
});

afterEach(() => {
fs.writeFileSync(parcelPkgPath, parcelPkg);
});

jest.mock('child_process', () => ({
spawnSync: jest.fn((_cmd: string, args: string[]) => {
if (args[1] === 'error') {
if (args.includes('/entry/error')) {
return { error: 'parcel-error' };
}

if (args[1] === 'status') {
if (args.includes('/entry/status')) {
return { status: 1, stdout: Buffer.from('status-error') };
}

return { error: null, status: 0 };
})
}));

test('calls parcel with the correct args', () => {
test('calls docker with the correct args', () => {
const builder = new Builder({
entry: 'entry',
global: 'handler',
Expand All @@ -36,18 +25,29 @@ test('calls parcel with the correct args', () => {
});
builder.build();

expect(spawnSync).toHaveBeenCalledWith(expect.stringContaining('parcel-bundler'), expect.arrayContaining([
'build', 'entry',
'--out-dir', 'out-dir',
// docker build
expect(spawnSync).toHaveBeenNthCalledWith(1, 'docker', [
'build', '-t', 'parcel-bundler', path.join(__dirname, '../parcel-bundler')
]);

// docker run
expect(spawnSync).toHaveBeenNthCalledWith(2, 'docker', [
'run', '--rm',
'-v', `${path.join(__dirname, '..')}:/entry`,
'-v', `${path.join(__dirname, '../out-dir')}:/out`,
'-v', `${path.join(__dirname, '../cache-dir')}:/cache`,
'parcel-bundler',
'parcel', 'build', '/entry/entry',
'--out-dir', '/out',
'--out-file', 'index.js',
'--global', 'handler',
'--target', 'node',
'--bundle-node-modules',
'--log-level', '2',
'--no-minify',
'--no-source-maps',
'--cache-dir', 'cache-dir'
]));
'--cache-dir', '/cache'
]);
});

test('throws in case of error', () => {
Expand All @@ -67,15 +67,3 @@ test('throws if status is not 0', () => {
});
expect(() => builder.build()).toThrow('status-error');
});

test('throws when parcel-bundler is not 1.x', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for when docker is not installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fs.writeFileSync(parcelPkgPath, JSON.stringify({
...JSON.parse(parcelPkg.toString()),
version: '2.3.4'
}));
expect(() => new Builder({
entry: 'entry',
global: 'handler',
outDir: 'out-dur'
})).toThrow(/This module has a peer dependency on parcel-bundler v1.x. Got v2.3.4./);
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"Properties": {
"Code": {
"S3Bucket": {
"Ref": "AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543S3BucketD096DC83"
"Ref": "AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eS3Bucket9E60CFAE"
},
"S3Key": {
"Fn::Join": [
Expand All @@ -49,7 +49,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543S3VersionKeyA76287A5"
"Ref": "AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eS3VersionKey74429E96"
}
]
}
Expand All @@ -62,7 +62,7 @@
"Fn::Split": [
"||",
{
"Ref": "AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543S3VersionKeyA76287A5"
"Ref": "AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eS3VersionKey74429E96"
}
]
}
Expand Down Expand Up @@ -172,17 +172,17 @@
}
},
"Parameters": {
"AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543S3BucketD096DC83": {
"AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eS3Bucket9E60CFAE": {
"Type": "String",
"Description": "S3 bucket for asset \"a736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543\""
"Description": "S3 bucket for asset \"fdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88e\""
},
"AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543S3VersionKeyA76287A5": {
"AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eS3VersionKey74429E96": {
"Type": "String",
"Description": "S3 key for asset version \"a736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543\""
"Description": "S3 key for asset version \"fdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88e\""
},
"AssetParametersa736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543ArtifactHashB7337532": {
"AssetParametersfdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88eArtifactHashAA5D4787": {
"Type": "String",
"Description": "Artifact hash for asset \"a736580e4382e6d67a7acaedbbd271f559ed918382808ae1431364ae70286543\""
"Description": "Artifact hash for asset \"fdefbd5f0d90231e11e34991218a3674ec3ac1a3a99948038164c6d7a8efa88e\""
},
"AssetParameters0d445d366e339b4344917ba638a4cb2dcddfd0e063b10fb909340fd1cc51c278S3BucketDBD288E6": {
"Type": "String",
Expand Down