-
Notifications
You must be signed in to change notification settings - Fork 154
fix: Enable --manifest flag for esbuild and npm build #513
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
Conversation
mildaniel
left a comment
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.
Thanks Mohamed, this looks great! Just one question.
| CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES), | ||
| ] | ||
|
|
||
| if is_external_manifest: |
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.
I'm a little confused by this. I thought that today this feature worked for regular Node.js builds.
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.
not sure if it was a bug or a regression, but I verified that it is not working for Node. Build will succeed, but actually if you try to run sam local invoke it will fail, as the source code does not exist in the artifacts directory
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.
$ sam build --manifest manifest/package.json
SAM CLI now collects telemetry to better understand customer needs.
You can OPT OUT and disable telemetry collection by setting the
environment variable SAM_CLI_TELEMETRY=0 in your shell.
Thanks for your help!
Learn More: https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-telemetry.html
Building codeuri: /Volumes/workplace/sam-apps/bug_4832/sam-app-node/hello-world runtime: nodejs18.x metadata: {} architecture: x86_64
functions: HelloWorldFunction
Running NodejsNpmBuilder:NpmPack
Running NodejsNpmBuilder:CopyNpmrcAndLockfile
Running NodejsNpmBuilder:CopySource
Running NodejsNpmBuilder:NpmInstall
Running NodejsNpmBuilder:CleanUpNpmrc
Running NodejsNpmBuilder:LockfileCleanUp
Build Succeeded
Built Artifacts : .aws-sam/build
Built Template : .aws-sam/build/template.yaml
Commands you can use next
=========================
[*] Validate SAM template: sam validate
[*] Invoke Function: sam local invoke
[*] Test Function in the Cloud: sam sync --stack-name {{stack-name}} --watch
[*] Deploy: sam deploy --guided
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.
$ sam local invoke
Invoking app.lambdaHandler (nodejs18.x)
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.
Mounting /Volumes/workplace/sam-apps/bug_4832/sam-app-node/.aws-sam/build/HelloWorldFunction as /var/task:ro,delegated, inside runtime
container
START RequestId: ed901fe3-6a35-4cfd-a70c-7a7e4d4a7ea0 Version: $LATEST
2023-06-07T17:10:20.910Z undefined ERROR Uncaught Exception {"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","stack":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs"," at _loadUserApp (file:///var/runtime/index.mjs:997:17)"," at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)"," at async start (file:///var/runtime/index.mjs:1195:23)"," at async file:///var/runtime/index.mjs:1201:1"]}
07 Jun 2023 17:10:21,025 [ERROR] (rapid) Init failed error=Runtime exited with error: exit status 129 InvokeID=
2023-06-07T17:10:22.198Z undefined ERROR Uncaught Exception {"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","stack":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs"," at _loadUserApp (file:///var/runtime/index.mjs:997:17)"," at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)"," at async start (file:///var/runtime/index.mjs:1195:23)"," at async file:///var/runtime/index.mjs:1201:1"]}
END RequestId: 9aa21ed6-ab13-44df-a7a3-0b1bc26adb8b
REPORT RequestId: 9aa21ed6-ab13-44df-a7a3-0b1bc26adb8b Init Duration: 1.00 ms Duration: 2689.49 ms Billed Duration: 2690 ms Memory Size: 128 MB Max Memory Used: 128 MB
{"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","trace":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs"," at _loadUserApp (file:///var/runtime/index.mjs:997:17)"," at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)"," at async start (file:///var/runtime/index.mjs:1195:23)"," at async file:///var/runtime/index.mjs:1201:1"]}
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.
but if I used the new lambda builders library:
$ samdev build --manifest manifest/package.json
2023-06-07 10:11:23 Attaching import module proxy for analyzing dynamic imports
Building codeuri: /Volumes/workplace/sam-apps/bug_4832/sam-app-node/hello-world runtime: nodejs18.x metadata: {} architecture: x86_64
functions: HelloWorldFunction
Running NodejsNpmBuilder:NpmPack
Running NodejsNpmBuilder:CopyNpmrcAndLockfile
Running NodejsNpmBuilder:CopySource
Running NodejsNpmBuilder:CopySource
Running NodejsNpmBuilder:NpmInstall
Running NodejsNpmBuilder:CleanUpNpmrc
Running NodejsNpmBuilder:LockfileCleanUp
Build Succeeded
Built Artifacts : .aws-sam/build
Built Template : .aws-sam/build/template.yaml
Commands you can use next
=========================
[*] Validate SAM template: sam validate
[*] Invoke Function: sam local invoke
[*] Test Function in the Cloud: sam sync --stack-name {{stack-name}} --watch
[*] Deploy: sam deploy --guided
$ sam local invoke
Invoking app.lambdaHandler (nodejs18.x)
Local image is up-to-date
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.
Mounting /Volumes/workplace/sam-apps/bug_4832/sam-app-node/.aws-sam/build/HelloWorldFunction as /var/task:ro,delegated, inside runtime
container
START RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3 Version: $LATEST
END RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3
REPORT RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3 Init Duration: 1.02 ms Duration: 2961.36 ms Billed Duration: 2962 ms Memory Size: 128 MB Max Memory Used: 128 MB
{"statusCode":200,"body":"{\"message\":\"hello world\"}"}
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.
so I also added some integration test cases to SAM CLI to cover this use case to avoid regressions in the future if it was a regression.
| if is_building_in_source and is_external_manifest: | ||
| # Since we run `npm install` in the manifest directory, so we need to link the node_modules directory in | ||
| # the source directory. | ||
| source_dependencies_path = os.path.join(self.source_dir, "node_modules") |
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.
Nice! This is a clever solution.
lucashuy
left a comment
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.
LGTM, thanks for testing the different cases
Issue #, if available:
aws/aws-sam-cli#4832, I also found that this issue exist in the NPM build.
Description of changes:
For NPM Build:
node_modulesfrom the manifest directory to the source code directory.For ESBuild:
node_modulesfrom the manifest directory to the source code directory.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.