Add ephemeralStorageSizeMB option to defineFunction #2283
Conversation
🦋 Changeset detectedLatest commit: 847124a 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 |
There was a problem hiding this comment.
Thanks for taking the time to file this @fossamagna ! Change looks good to me from a dx perspective
packages/backend-function/API.md
Outdated
| @@ -93,6 +93,7 @@ export type FunctionProps = { | |||
| entry?: string; | |||
| timeoutSeconds?: number; | |||
| memoryMB?: number; | |||
| ephemeralStorageSize?: number; | |||
There was a problem hiding this comment.
Can we suffix this with MB ? we do that in property above.
There was a problem hiding this comment.
Some context:
It seems that CDK is just passing through this value:
see https://github.com/aws/aws-cdk/blob/b5c21890cf67ddda78c18eb0425f11a59fcadb34/packages/aws-cdk-lib/aws-lambda/lib/function.ts#L1057
and perhaps multiplies it by 1024 if necessary.
see https://github.com/aws/aws-cdk/blob/62a0638e325024fc0469ed50f9accd448d87bac7/packages/aws-cdk-lib/core/lib/size.ts#L133
But the lambda claims it's MB not MiB here https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-ephemeralstorage.html .
To confirm this. It might be good to deploy lambda with ephemeralStorageSize given to CDK construct and checking what value is printed in the AWS Console.
There was a problem hiding this comment.
@sobolk
In CloudFormation and AWS Console, the unit of ehpemeralStorageSize for Lambda was MB.
I understood that only CDK uses MiB as a unit and it is appropriate to give MB to the suffix in Amplify.
I fixed the part of JSDoc for ephemeralStorageSizeMB that used MiB has been corrected to MB.
Thank you very much.
packages/client-config/API.md
Outdated
| @@ -627,7 +627,7 @@ export type CustomClientConfig = { | |||
| export const DEFAULT_CLIENT_CONFIG_VERSION: ClientConfigVersion; | |||
|
|
|||
| // @public | |||
| export const generateClientConfig: <T extends "1" | "1.1" | "1.2" | "1.3" | "0">(backendIdentifier: DeployedBackendIdentifier, version: T, awsClientProvider?: AWSClientProvider<{ | |||
| export const generateClientConfig: <T extends "1.3" | "0" | "1" | "1.1" | "1.2">(backendIdentifier: DeployedBackendIdentifier, version: T, awsClientProvider?: AWSClientProvider<{ | |||
There was a problem hiding this comment.
the changes in packages/client-config/API.md and packages/platform-core/API.md are unexpected.
You can either revert these two files.
Or run npm run clean && npm install && npm run build && npm update:api . I.e. re-generate api reports from clean workspace.
fossamagna
changed the title
| ephemeralStorageSizeMax | ||
| ) | ||
| ) { | ||
| throw new Error( |
There was a problem hiding this comment.
Please throw an AmplifyUserError instead of a regular error. See line 327 for an example
https://github.com/aws-amplify/amplify-backend/pull/2283/files#diff-e354c6c4617ce06e0e16948f4521eca5f9269445265b80ed16720372d336c886R327
Problem
Issue number, if available:
#2282
Changes
Corresponding docs PR, if applicable:
aws-amplify/docs#8142
Validation
Added test.
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.