feat(ecs): add support for Fargate PV1.4 ephemeral storage#15440
feat(ecs): add support for Fargate PV1.4 ephemeral storage#15440mergify[bot] merged 2 commits intoaws:masterfrom
Conversation
|
|
ea7b87e to
bf6d38e
Compare
|
Any ideas when this will get released? |
upparekh
left a comment
There was a problem hiding this comment.
Thanks so much for adding support for this feature! Just a few nits..
| /** | ||
| * The amount (in GiB) of ephemeral storage to be allocated to the task. | ||
| * | ||
| * Only supported in Fargate platform version 1.4.0 or later. | ||
| */ | ||
| public readonly ephemeralStorageGiB?: number; | ||
|
|
There was a problem hiding this comment.
Usually an input property would be added as a class attribute if it can possibly be referred to later in the code. I'm curious if there would be such a use case for ephemeral storage for which we need to add it here.
There was a problem hiding this comment.
I've run into a number of issues in the past with multiple L2 constructs where their input values were swallowed by the constructor and made inaccessible after that. This poses significant challenges to classes and methods that consume the resulting construct and need to examine it. So I am now making these attributes readable by default, unless there's some really good reason to hide them.
We simply cannot predict the future with certainty as to when these attributes might someday be useful to consumers. It's always easier to make them accessible from the beginning, just in case, rather than hide them by default and then have to undo our mistake later.
There was a problem hiding this comment.
Hey @otterley, CDK team member weighing in here. In this case, I don't think it is necessary or helpful to make this a public class attribute, since the ephemeralStorageGiB value will be undefined if the user did not provide it in the input props. If the user provided this value to the constructor, then they should be able to design their CDK app in such a way that they have access to that value wherever they need it. In the case where the user does not provide a value here, they may be frustrated that ephemeralStorageGiB is undefined, and not informing them what the default value is.
I am willing to disagree and commit here. Since this is an optional attribute, it is ultimately a 2-way door decision. So, the decision is ultimately up to you.
| /** | ||
| * The amount (in GiB) of ephemeral storage to be allocated to the task. | ||
| */ | ||
| public readonly ephemeralStorageGiB?: number; | ||
|
|
There was a problem hiding this comment.
Same suggestion as before, we can possibly remove adding it as a class attribute here as well.
| networkMode: NetworkMode.AWS_VPC, | ||
| }); | ||
|
|
||
| if (props.ephemeralStorageGiB && (props.ephemeralStorageGiB < 20 || props.ephemeralStorageGiB > 200)) { |
There was a problem hiding this comment.
The condition should be:
if (props.ephemeralStorageGiB && (props.ephemeralStorageGiB < 21 || props.ephemeralStorageGiB > 200))
as the current statement would make the ephemeral storage range 20-200 while we want it to be 21-200.
| const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TaskDef', { | ||
| ephemeralStorageGiB: 100, | ||
| }); |
There was a problem hiding this comment.
Thanks for being so thorough with the tests! I think the edge cases are covered in the unit tests and perhaps we don't need an integ test for this feature.
| * | ||
| * @default - 20GiB | ||
| */ |
There was a problem hiding this comment.
I think we can be more clear about the default value here given that we are not setting it unless provided by the user. Something like 'None, the task will receive 20 GiB of ephemeral storage by default.'
bf6d38e to
b337298
Compare
b337298 to
63dd00c
Compare
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Add support for ephemeral storage on Fargate PV 1.4.0 or later. Closes aws#14570 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for ephemeral storage on Fargate PV 1.4.0 or later. Closes aws#14570 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add support for ephemeral storage on Fargate PV 1.4.0 or later.
Closes #14570
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license