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(apigateway): SAM CLI asset metadata missing from SpecRestApi #17293

Merged
merged 12 commits into from Nov 11, 2021

Conversation

moelasmar
Copy link
Contributor

@moelasmar moelasmar commented Nov 3, 2021

Adds Assets metadata to RestApi resource in case if AssetApiDefinition is used. This Metadata will enable SAM
CLI to find local assets used by RestApi in the template.

It follows the same design in document design/code-asset-metadata.md

Fixes #14593


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

@gitpod-io
Copy link

gitpod-io bot commented Nov 3, 2021

@moelasmar moelasmar changed the title feat(AppiGateway): add Asset metadata to SpecRestApi resource in case of using AssetApiDefinition fix(AppiGateway): add Asset metadata to SpecRestApi resource in case of using AssetApiDefinition Nov 3, 2021
@peterwoodworth peterwoodworth changed the title fix(AppiGateway): add Asset metadata to SpecRestApi resource in case of using AssetApiDefinition fix(AppiGateway): add Asset metadata to SpecRestApi resource in case of using AssetApiDefinition Nov 3, 2021
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Nov 3, 2021
nija-at
nija-at previously requested changes Nov 5, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Can you explain what is going on with the changes to cdk-integ?

@moelasmar
Copy link
Contributor Author

The integration between SAM and CDK was implemented before using this design. The idea is SAM needs to know the local path of the assets attached to Lambda Function so SAM can implement the local testing and debugging functionalities. The solution was to let CDK add metadata properties for the Lambda Function resources to let SAM know the code path.

In This PR we want to extend this solution to SpecRestApi resource to add the assets metadata to it in case if it is using AssetApiDefinition.

@moelasmar
Copy link
Contributor Author

moelasmar commented Nov 5, 2021

in this PR .. I also updated the Lambda function runtime from NodeJs10 to NodeJs14 to fix the integration testing failures.

* Definition to bind to it. Specifically it's required to allow assets to add
* metadata for tooling like SAM CLI to be able to find their origins.
*/
public bindToResource(_resource: CfnResource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this: bindAfterCreate(scope: Construct, restApi: RestApi) ?

Call it like apiDefinition.bindAfterCreate(this, this), and you can access the CfnResource inside like:

const child = Node.of(restApi).defaultChild as CfnRestApi;
child.addMetadata(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@mergify mergify bot dismissed rix0rrr’s stale review November 11, 2021 09:35

Pull request has been modified.

@rix0rrr rix0rrr changed the title fix(AppiGateway): add Asset metadata to SpecRestApi resource in case of using AssetApiDefinition fix(apigateway): SAM CLI asset metadata missing from SpecRestApi Nov 11, 2021
rix0rrr
rix0rrr previously approved these changes Nov 11, 2021
@mergify mergify bot dismissed rix0rrr’s stale review November 11, 2021 09:47

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2021

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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 5673747
  • 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 841cf99 into aws:master Nov 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 11, 2021

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

mergify bot pushed a commit that referenced this pull request Nov 15, 2021
)

Adds asset metadata to image-type lambda functions. This will allow SAM CLI to support local invocation of image-type lambdas from CDK-synthed templates.

It follows the same design and builds upon #1433

Fixes #14593

Uses some changes from #17293 to enable asset metadata generation in integration tests

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mpvosseller pushed a commit to mpvosseller/aws-cdk that referenced this pull request Nov 16, 2021
…#17368)

Adds asset metadata to image-type lambda functions. This will allow SAM CLI to support local invocation of image-type lambdas from CDK-synthed templates.

It follows the same design and builds upon aws#1433

Fixes aws#14593

Uses some changes from aws#17293 to enable asset metadata generation in integration tests

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…#17293)

Adds Assets metadata to RestApi resource in case if AssetApiDefinition is used. This Metadata will enable SAM
CLI to find local assets used by RestApi in the template.

It follows the same design in document [design/code-asset-metadata.md](https://github.com/aws/aws-cdk/pull/design/code-asset-metadata.md)

Fixes aws#14593

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…#17368)

Adds asset metadata to image-type lambda functions. This will allow SAM CLI to support local invocation of image-type lambdas from CDK-synthed templates.

It follows the same design and builds upon aws#1433

Fixes aws#14593

Uses some changes from aws#17293 to enable asset metadata generation in integration tests

*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
@aws-cdk/aws-apigateway Related to Amazon API Gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(assets, aws-lambda): Add asset metadata for nested stack and image-type Lambda Function
4 participants