Add MemorySize validation for function build
#6679
Conversation
There was a problem hiding this comment.
Adding validation in build doesn't seem to me that it will resolve the issue customer described. sam build is not a prerequisite of sam local invoke (e.g. when the Lambda code has no external dependency at all or when customer has their own external build steps). sam local * seems to be the place where an incorrect config would fail invocation and thus needs validation.
I also wonder if we want to add our own validation logic. It's a duplication of knowledge as we already have sam validate --lint which would catch config errors. We can update sam local * to run sam validate --lint before starting to do the local stuff.
samcli/lib/build/app_builder.py
Outdated
| MIN_MEMORY_SIZE = 128 | ||
| MAX_MEMORY_SIZE = 10240 | ||
|
|
||
| if function.memory: | ||
| try: | ||
| function_memory = int(function.memory) | ||
| except ValueError as ex: | ||
| raise InvalidFunctionDefinition( | ||
| f'Function {function.function_id} has invalid MemorySize "{function.memory}". Must be an integer between 128 and 10240.' | ||
| ) from ex | ||
|
|
||
| if function_memory < MIN_MEMORY_SIZE or function_memory > MAX_MEMORY_SIZE: | ||
| raise InvalidFunctionDefinition( | ||
| f"Function {function.function_id} has MemorySize {function.memory}, but it must be an integer between 128 and 10240." | ||
| ) |
There was a problem hiding this comment.
Can we give a warning message rather than failing with the exception here? My concern is if this is changed and we missed it, then customers will be blocked building their lambda functions.
There was a problem hiding this comment.
Good point, however with Wing's comment we'll only be relying on cfnlint which we can expect to stay in sync with the CFN rules?
In align with @hawflau's feedback here, I think this validation should be done at I wouldn't add a validation logic to |
|
I've refactored to run the Integ tests to follow. |
| @@ -19,6 +19,13 @@ | |||
| from ..invoke.layer_utils import LayerUtils | |||
|
|
|||
|
|
|||
| # class TestInvalidTempate(StartApiIntegBaseClass): | |||
There was a problem hiding this comment.
nit: remove the lines if they are not needed
There was a problem hiding this comment.
Oops, didn't mean to commit this
|
Closing in favour of new PR on separate branch |
Validate function
MemorySizeto be an integer within range[128, 10240]as per docs.Also had to fix a bunch of unit tests that previously had fake MemorySize.
Which issue(s) does this change fix?
#6510
Why is this change necessary?
Customers were permitted to build Lambda functions with invalid MemorySize, causing late/unexpected failure
How does it address the issue?
Adds a validation step to tell the customer the MemorySize is invalid (not an integer, or outside the range
[128,10240])What side effects does this change have?
N/A
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.