-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ecs): add support for Fargate PV1.4 ephemeral storage #15440
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,15 @@ export interface FargateTaskDefinitionProps extends CommonTaskDefinitionProps { | |
* @default 512 | ||
*/ | ||
readonly memoryLimitMiB?: number; | ||
|
||
/** | ||
* The amount (in GiB) of ephemeral storage to be allocated to the task. The maximum supported value is 200 GiB. | ||
* | ||
* NOTE: This parameter is only supported for tasks hosted on AWS Fargate using platform version 1.4.0 or later. | ||
* | ||
* @default 20 | ||
*/ | ||
readonly ephemeralStorageGiB?: number; | ||
} | ||
|
||
/** | ||
|
@@ -104,6 +113,11 @@ export class FargateTaskDefinition extends TaskDefinition implements IFargateTas | |
// we need to explicitly write the type here, as type deduction for enums won't lead to | ||
// the import being generated in the .d.ts file. | ||
|
||
/** | ||
* The amount (in GiB) of ephemeral storage to be allocated to the task. | ||
*/ | ||
public readonly ephemeralStorageGiB?: number; | ||
|
||
Comment on lines
+116
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same suggestion as before, we can possibly remove adding it as a class attribute here as well. |
||
/** | ||
* Constructs a new instance of the FargateTaskDefinition class. | ||
*/ | ||
|
@@ -115,5 +129,11 @@ export class FargateTaskDefinition extends TaskDefinition implements IFargateTas | |
compatibility: Compatibility.FARGATE, | ||
networkMode: NetworkMode.AWS_VPC, | ||
}); | ||
|
||
if (props.ephemeralStorageGiB && (props.ephemeralStorageGiB < 21 || props.ephemeralStorageGiB > 200)) { | ||
throw new Error('Ephemeral storage size must be between 21GiB and 200GiB'); | ||
} | ||
|
||
this.ephemeralStorageGiB = props.ephemeralStorageGiB; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatephemeralStorageGiB
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.