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.

Sign up for GitHub

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

Jump to bottom

fix(core) id conflict with multiple buildLambdaFunction() calls #910

Merged
merged 1 commit into from
Mar 2, 2023
Merged

fix(core) id conflict with multiple buildLambdaFunction() calls #910

merged 1 commit into from
Mar 2, 2023

Conversation

alsoknownasdrew
Copy link
Contributor

@alsoknownasdrew alsoknownasdrew commented Feb 24, 2023 •
edited
Loading

Description of changes:

Currently multiple calls to buildLambdaFunction() with lambdaFunctionProps in the same stack throws the following error:

There is already a Construct with name 'LambdaFunctionServiceRole' in Stack [Default]

Error is caused by a call to deployLambdaFunction() with undefined as function id. So deployLambdaFunction creates a lambda with the same default id for every call.

This PR fixes the issue by passing lambdaFunctionProps.functionName as lambda id to deployLambdaFunction() and adds 2 new tests to lambda-helper.test.ts:

  • 'test buildLambdaFunction with lambdaFunctionProps default id' that assures that current behaviour is preserved (when lambdaFunctionProps.functionName is not set a default id will be used)
  • 'test buildLambdaFunction with lambdaFunctionProps custom id' that assures that lambdaFunctionProps.functionName is used when present

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: df9d6a4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@alsoknownasdrew alsoknownasdrew force-pushed the build-lambda-function-id branch from df9d6a4 to c760e2b Compare February 24, 2023 10:46
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: githubautobuild-for-cdk-v2
  • Commit ID: c760e2b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@biffgaut
Copy link
Contributor

Thanks for the PR! We're having trouble producing the issue this is addressing, can you post code that triggers the bad behavior?

@alsoknownasdrew
Copy link
Contributor Author

Sure I'll prepare a gist with code to reproduce it tomorrow, thanks 👍

@alsoknownasdrew
Copy link
Contributor Author

Hi! I added a sample repository here with the instructions to reproduce the issue: https://github.com/alsoknownasdrew/build-lambda-function-reproduction Let me know if other info is needed ;)

@biffgaut biffgaut merged commit 0fdeea4 into awslabs:main Mar 2, 2023
@biffgaut
Copy link
Contributor

biffgaut commented Mar 2, 2023

Looks good, thanks. This will get published to npm/pypi/etc. upon the next release.

@biffgaut
Copy link
Contributor

biffgaut commented Mar 4, 2023

This was released in v2.33.0 on 5/3/2023.

I should point out that calling the non-documented functions in core assumes a risk, as they can change outside of the scope of a major release.

georgebearden pushed a commit that referenced this pull request Mar 23, 2023
@alsoknownasdrew @georgebearden
fix(core) prevent lambda id conflict with multiple buildLambdaFunctio…
824b57f 
…n() calls in the same stack (#910)
biffgaut added a commit that referenced this pull request Jul 26, 2023
@georgebearden @alsoknownasdrew
@aws-solutions-constructs-team @biffgaut
feat(new construct): aws-openapigateway-lambda (#912)
09465d6 
* README for new openapi-based apigatway-to-lambda construct.

* fix(core) prevent lambda id conflict with multiple buildLambdaFunction() calls in the same stack (#910)

* chore(release): 2.33.0

* chore(release prep): Update CHANGELOG.md and align-version.js

* Freeze @types/node version in cdk-integ

* Freeze @types/node version in cdk-integ

* Update step functions integ tests

* chore(core): Add warnings about using core functions from outside of Solutions Constructs (#917)

* Add file header warning

* Add function header comments

* Use latest CDK to address .NET version issue

* Fix 2 typos

* One last typo

* fix(StepFunctions): Address LogGroup behavior problems (#922)

* Implementation

* Clean up some code cruft

* Update package.json

* chore(release): 2.34.0

* chore(release-prep): Updated CHANGELOG.md and align-version.js

* chore(release-prep): align-version.js

* chore(core): migrate to assertions (#929)

* First two test updates

* chore(core): migrate to assertions

* Remove old lib, final few modules

* Update the cloudfront-to-s3 construct to correctly set the logging bucket property. (#930)

* Update README

* Update openapigateway-to-lambda README

* Update README/architecture for openapigateway-to-lambda.

* Add openapigateway-to-lambda code and initial tests

* update openapigateway-to-lambda package.json dependency versions.

* Update openapigateway-to-lambda

* Update openapigateway README

* don't depend on NodeJsFunction docker env

* Update openapigateway-to-lambda README to reflect actual construct API.

* temp commit

* Update snapshot test for openapigateway-to-lambda construct.

* fix package.json version field

* update snapshot

* update openapigateway-to-lambda custom resource to suppress standard cfn nag warnings.

* [wip] resources project

* fix dependency self reference on new resources module

* fix dependency self reference on new resources module

* Add integ test to the template writer resource.

* Add integ tests to template-writer resource.

* Update template-writer resource integ tests to clean up test buckets automatically

* Add additinal tests to get 100% coverage on aws-openapigateway-lambda

* Add additional integration tests to template resource writer and aws-openapigateway-lambda.

* remove eslintignore line that was obsolete

* cleanup eslint ignore and update openapigateway-to-lambda props.

* Update python/java code samples for openapigateway-to-lambda.

* Update openapigateway-to-lambda README

* Update resources README

* Update resources integ test

* update resources integ test snapshot

* Update integ test snapshots for aws-openapigateway-lambda.

* Update aws-openapigateway-lambda construct to trigger api deployments anytime the incoming api template changes or any time the lambda functions change.

* Add new integ test for cognito authorizer on aws-openapigateway-lambda construct.

* address minor pr feedback.

* Remove integration tests that use BucketDeployment as the asset hash of the AwsCliLayer changes outside our control.

* Remove integration tests that use BucketDeployment as the asset hash of the AwsCliLayer changes outside our control.

* Address pr feedback.

Add new template writer integration test for transforming large template with several thousand substitutions
Several new comments/documentation to make the obscure less so, hopefully>

* Add additional tests to aws-openapigateway-lambda construct.

* Update integ tests after latest cdk lib update.

* Add optional construct id parameter to the buildLambdaFunction function.

* Update aws-openapigateway-lambda property descriptions to better explain optional custom resource values.

---------

Co-authored-by: Andriy <andriy.frankevych@gmail.com>
Co-authored-by: AWS Solutions Constructs Automation <aws-solutions-constructs-team@amazon.com>
Co-authored-by: biffgaut <biffgaut@amazon.com>
Co-authored-by: biffgaut <78155736+biffgaut@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biffgaut biffgaut biffgaut approved these changes

@hayesry hayesry Awaiting requested review from hayesry hayesry is a code owner

@mobri2a mobri2a Awaiting requested review from mobri2a mobri2a is a code owner

@emcfins emcfins Awaiting requested review from emcfins emcfins is a code owner

@mickychetta mickychetta Awaiting requested review from mickychetta

@georgebearden georgebearden Awaiting requested review from georgebearden georgebearden is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alsoknownasdrew @aws-solutions-constructs-team @biffgaut