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

feat(lambda): inline function code can exceed 4096 bytes #20624

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Conversation

seyeong
Copy link
Contributor

@seyeong seyeong commented Jun 6, 2022

CloudFormation removed the 4k charcaters limit for inline function code.

Latest public documentation: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#cfn-lambda-function-code-zipfile

Historic documentation: https://web.archive.org/web/20220309033724/https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#cfn-lambda-function-code-zipfile

Internal Code Review reference: CR-53662636


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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 Jun 6, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team June 6, 2022 00:20
@github-actions github-actions bot added the p2 label Jun 6, 2022
@seyeong seyeong marked this pull request as ready for review June 6, 2022 01:19
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Thanks for the documentation on this change @seyeong. Just to be sure, I would like to test this in an integ test (maybe in integ.lambda.ts) just to be sure that it will deploy with > 4096 bytes

@seyeong
Copy link
Contributor Author

seyeong commented Jun 7, 2022

@kaizencc Can you please help me understand why you're requesting an integer test? I don't see a point adding it.

@kaizencc
Copy link
Contributor

kaizencc commented Jun 7, 2022

@seyeong for peace of mind. you're loosening a behavior that previously would have resulted in a failed deployment. it stands that we should check that the deployment will not fail before removing the check.

@kaizencc kaizencc changed the title feat(lambda): Remove inline code limit feat(lambda): inline function code can exceed 4096 bytes Jun 7, 2022
@seyeong
Copy link
Contributor Author

seyeong commented Jun 11, 2022

it stands that we should check that the deployment will not fail before removing the check.

The check can be done manually. To me, it feels like we're producing a tech debt by add it to the integration test. I personally test it myself and you can do it as well. If you still think that it is necessary to add an automated test, let me know.

@mergify mergify bot dismissed kaizencc’s stale review June 13, 2022 18:56

Pull request has been modified.

kaizencc
kaizencc previously approved these changes Jun 13, 2022
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

The important thing is to have tested a deployment of this code (beyond what a unit test can do). Documenting that you have done so on this PR is enough for me.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

We need an update to the README for this change. fromInline still documents this limit. Please also verify that you have manually tested this change and we can remove the label requiring the integration test.

@mergify mergify bot dismissed stale reviews from kaizencc and TheRealAmazonKendra June 14, 2022 04:29

Pull request has been modified.

@TheRealAmazonKendra TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jun 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 14, 2022

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: 6f63ce2
  • 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 a014c30 into aws:main Jun 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 14, 2022

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

mergify bot pushed a commit that referenced this pull request Jun 14, 2022
Fixing comment. The actual limit is 5MB according to [CloudFormation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-synthetics-canary-code.html#cfn-synthetics-canary-code-script). There's no tests verifying the limit.

Related to #20624

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)


*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
CloudFormation removed the 4k charcaters limit for inline function code.

Latest public documentation: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#cfn-lambda-function-code-zipfile

Historic documentation: https://web.archive.org/web/20220309033724/https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html#cfn-lambda-function-code-zipfile

Internal Code Review reference: CR-53662636


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
Fixing comment. The actual limit is 5MB according to [CloudFormation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-synthetics-canary-code.html#cfn-synthetics-canary-code-script). There's no tests verifying the limit.

Related to aws#20624

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)


*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
p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants