-
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
fix(lambda-nodejs): Required auto prefix of handler
with index.
breaks custom non-index
handler settings used by layers
#24406
fix(lambda-nodejs): Required auto prefix of handler
with index.
breaks custom non-index
handler settings used by layers
#24406
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
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.
This is an elegant solution, well done! Just one comment on the integration test, but this looks very close to merge
sourceMap: true, | ||
sourceMapMode: lambda.SourceMapMode.BOTH, | ||
}, | ||
handler: 'run.sh', |
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.
Is there a way we can make this example not only deploy but actually function if invoked? run.sh
doesn't exist in this directory, but it seems like a good place to add one of the lambda extensions you referenced
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, yeah that is simple enough.
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 have added code that makes the handler override work (well... it doesn't setup a Function URL or API Gateway for the Lambda, but if it did, it should work) and confirmed it deploys.
Let me know if this is what you were looking for as it could be a bit much.
Please update the title of the PR to describe the issue you're fixing, not how you're fixing it |
handler
when it contains a .
handler
with index.
breaks custom non-index
handler settings used by layers
How is the new title? |
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, nice work!
Thank you for contributing! Your pull request will be updated from main 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…reaks custom non-`index` handler settings used by layers (aws#24406) Using `lambda-nodejs` makes it very easy to bundle functions with `esbuild`, but the code currently *always* prefixes the `handler` value with `index.`, which makes it impossible to use some lambda extensions together with this module as they require setting `handler` to specific values and the `index.` prefixing breaks the ability to set the handler to those values. This PR avoids adding the `index.` prefix if the specified `handler` value contains a `.` already. Closes aws#24403 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Using
lambda-nodejs
makes it very easy to bundle functions withesbuild
, but the code currently always prefixes thehandler
value withindex.
, which makes it impossible to use some lambda extensions together with this module as they require settinghandler
to specific values and theindex.
prefixing breaks the ability to set the handler to those values.This PR avoids adding the
index.
prefix if the specifiedhandler
value contains a.
already.Closes #24403
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license