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

fix(lambda-nodejs): Required auto prefix of handler with index. breaks custom non-index handler settings used by layers #24406

Merged
merged 4 commits into from
Mar 6, 2023

Conversation

huntharo
Copy link
Contributor

@huntharo huntharo commented Mar 2, 2023

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 #24403


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Mar 2, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team March 2, 2023 02:22
@github-actions github-actions bot added bug This issue is a bug. p2 labels Mar 2, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review March 2, 2023 02:37

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

Copy link
Contributor

@comcalvi comcalvi left a 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',
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 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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@huntharo huntharo Mar 4, 2023

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.

@comcalvi
Copy link
Contributor

comcalvi commented Mar 3, 2023

Please update the title of the PR to describe the issue you're fixing, not how you're fixing it

@huntharo huntharo changed the title fix(lambda-nodejs): Do not prefix handler when it contains a . fix(lambda-nodejs): Required auto prefix of handler with index. breaks custom non-index handler settings used by layers Mar 3, 2023
@huntharo
Copy link
Contributor Author

huntharo commented Mar 3, 2023

Please update the title of the PR to describe the issue you're fixing, not how you're fixing it

How is the new title?

@mergify mergify bot dismissed comcalvi’s stale review March 4, 2023 01:03

Pull request has been modified.

Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

lgtm, nice work!

@mergify
Copy link
Contributor

mergify bot commented Mar 6, 2023

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: b289bde
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit d7a1c34 into aws:main Mar 6, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 6, 2023

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).

@huntharo huntharo deleted the issue-24403/avoid-modifying-handler branch March 7, 2023 03:01
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-lambda-nodejs: Overrides of handler always get prefixed with index.
3 participants