-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
memory and timeout vars for lambda@edge #330
memory and timeout vars for lambda@edge #330
Conversation
📝 WalkthroughWalkthroughThe changes introduce two new configuration parameters, Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
examples/complete/lambda-at-edge.tf
(1 hunks)modules/lambda@edge/README.md
(1 hunks)modules/lambda@edge/main.tf
(1 hunks)modules/lambda@edge/variables.tf
(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
modules/lambda@edge/README.md
74-74: Element: a
Inline HTML
(MD033, no-inline-html)
75-75: Element: a
Inline HTML
(MD033, no-inline-html)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
modules/lambda@edge/main.tf (1)
85-86
: LGTM! Variables are properly integratedThe memory_size and timeout variables are correctly added to the aws_lambda_function resource.
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/complete/lambda-at-edge.tf (1)
35-36
: 🧹 Nitpick (assertive)Consider function-specific configurations.
While the current configuration works, consider adjusting memory and timeout based on function type:
- Viewer functions (request/response): Lower timeout (max 5s)
- Origin functions (request/response): Higher timeout if needed (max 30s)
Example configuration:
viewer_request = { ... memory_size = 128 - timeout = 3 + timeout = 5 # Maximum allowed for viewer functions ... } origin_response = { ... memory_size = 128 - timeout = 3 + timeout = 30 # Maximum allowed for origin functions ... }Also applies to: 45-46, 54-55, 88-89
modules/lambda@edge/README.md (1)
76-76
: 🧹 Nitpick (assertive)Enhance documentation with Lambda@Edge constraints.
Consider adding Lambda@Edge specific constraints to the documentation:
| <a name="input_functions"></a> [functions](#input_functions) | Lambda@Edge functions to create.<br><br>The key of this map is the name label of the Lambda@Edge function.<br><br>`source.filename` and `source.content` dictate the name and content of the files that will make up the Lambda function<br>source, respectively.<br><br>`runtime`, `handler`, `memory_size`, and `timeout` correspond to the attributes of the same name in the [lambda_function](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_function)<br>resource.<br><br>`event_type` and `include_body` correspond to the attributes of the same name in the [Lambda Function association block<br>of the cloudfront_distribution](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudfront_distribution#lambda-function-association)<br>resource. | +<br><br>Lambda@Edge specific constraints:<br> +- `memory_size`: Must be between 128 MB and 10240 MB<br> +- `timeout`: Must be between 1 and 30 seconds. Note that viewer-request and viewer-response functions have a 5-second timeout limit, while origin-request and origin-response functions have a 30-second limit. |🧰 Tools
🪛 Markdownlint (0.37.0)
76-76: Element: a
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: pre
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
76-76: Element: br
Inline HTML(MD033, no-inline-html)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md
(1 hunks)README.yaml
(1 hunks)examples/complete/lambda-at-edge.tf
(3 hunks)modules/lambda@edge/README.md
(1 hunks)modules/lambda@edge/main.tf
(1 hunks)modules/lambda@edge/variables.tf
(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
modules/lambda@edge/README.md
76-76: Element: a
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: pre
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
76-76: Element: br
Inline HTML
(MD033, no-inline-html)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
modules/lambda@edge/variables.tf (1)
32-33
: LGTM! Consider implementing the suggested validations.The addition of
memory_size
andtimeout
as optional parameters is correct. However, as suggested in previous review comments, consider adding validation blocks to enforce Lambda@Edge specific constraints:
- Memory: 128MB to 10240MB
- Timeout: 1-30s (with 5s limit for viewer functions)
modules/lambda@edge/main.tf (1)
81-82
: LGTM! Conservative default values.The implementation correctly sets conservative default values:
- memory_size = 128 (minimum allowed)
- timeout = 3 (safe for all function types)
README.yaml (1)
358-359
: LGTM! Example is consistent.The example configuration is consistent with the implementation and uses safe default values.
/terratest |
/terratest |
@joe-niland any idea why test are failing? |
/terratest |
@mihaiplesa I think it's due to SCP changes in the test account. I'll see if I can get an update. |
/terratest |
1 similar comment
/terratest |
/terratest |
Looks like tests finally passed, thanks @goruha. |
@mihaiplesa yes. But pls do not merge it yet. |
/terratest |
3 similar comments
/terratest |
/terratest |
/terratest |
@hans-d you're set as code owner on this, can you help by approving? Thank you in advance. |
These changes were released in v0.96.1. |
what
Allow to configure memory size and timeout for Lambda@Edge module.
why
These fields are not configurable now.
references
Resolves #331