add validation for environment prop #2290
Conversation
🦋 Changeset detectedLatest commit: f1242d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||
| Object.keys(this.props.environment).forEach((key) => { | ||
| // validate using key pattern from https://docs.aws.amazon.com/lambda/latest/api/API_Environment.html | ||
| if (!key.match(/^[a-zA-Z]([a-zA-Z0-9_])+$/) || key.length < 2) { |
There was a problem hiding this comment.
the length validation seems redundant with regex.
There was a problem hiding this comment.
Good catch, removed the length validation.
| entry: './test-assets/default-lambda/handler.ts', | ||
| name: 'myCoolLambda', | ||
| environment: { | ||
| ['this.is.wrong']: 'testValue', |
There was a problem hiding this comment.
a suggestion:
it might be worth running this in the loop with couple of wrong values.
for example ['a', 'this.is.wrong', ...]
reason is that. - we removed that length check, but we should still validate that regex catches 1 character violation.
There was a problem hiding this comment.
I have that captured in another unit test. But this got me thinking if we should validate all keys and list them in the error?
There was a problem hiding this comment.
But this got me thinking if we should validate all keys and list them in the error?
that's better DX.
| Object.keys(this.props.environment).forEach((key) => { | ||
| // validate using key pattern from https://docs.aws.amazon.com/lambda/latest/api/API_Environment.html | ||
| if (!key.match(/^[a-zA-Z]([a-zA-Z0-9_])+$/)) { | ||
| throw new Error( |
There was a problem hiding this comment.
Shouldn't this be AmplifyUserError?
There was a problem hiding this comment.
After comparing with other packages I believe you're right, though this error and others in FunctionFactory are caught by https://github.com/aws-amplify/amplify-backend/blob/main/packages/backend-deployer/src/cdk_error_mapper.ts#L352. I'll update all of them to AmplifyUserError.
Problem
Deployment can fail with the following error message when function environment keys are invalid:
Issue number, if available:
Changes
Add validation for
defineFunctionenvironment prop.Corresponding docs PR, if applicable:
Validation
Unit tests.
Checklist
run-e2elabel set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.