-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(lambda-nodejs): use docker instead of npm for parcel-bundler #7169
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Co-Authored-By: Jonathan Goldwasser <jogold@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I would like us to look into extracting this common code into a separate module like @aws-cdk/aws-lambda-bundling
. We can do that in a subsequent PR.
@@ -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', () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,5 @@ | |||
FROM node:13.8.0-alpine3.11 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,5 @@ | |||
FROM node:13.8.0-alpine3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@jogold LMK if this looks good. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I also wrote an implementation for Python that uses Docker. Feel free to use this when you make the separate module:
|
Would you like to contribute that? |
@eladb looks like Docker (docker-in-docker) is not available in the CodeBuild image that is running for PRs? |
I can't commit the time to that unfortunately. Feel free to use the code though! |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Damn! @RomainMuller any suggestion? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@AlexCheema can you update |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Done |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@AlexCheema have you been able to test this with dependencies/modules? I'm always getting |
I think that this is because the @eladb looking into this and waiting for a reply from @AlexCheema but I think that this needs to be reverted/not released... |
I'm not sure I understand the problem. Best if you implement a fix. |
|
@jogold Sorry, I was not more clear. I'm not aware of any process currently that knows if something in your lambda src files changed. So is there currently a way to use the output from parcel as the thing to generate the hash from? |
Commit Message
feat(lambda-nodejs): use docker instead of npm package for parcel-bundler
End Commit Message
There was a dependency on the
parcel-bundler
npm package for using the@aws-cdk/aws-lambda-nodejs
package. This required installing parcel-bundler in your project. This replaces the npm dependency with a docker image.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license