feat(batch): `ephemeralStorage` property on job definitions by sumupitchayan · Pull Request #25399 · aws/aws-cdk

sumupitchayan · 2023-05-02T19:03:46Z

Adds missing ephemeralStorage property to EcsFargateContainerDefinition and EcsFargateContainerDefinitionProps along with a unit test.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Signed-off-by: Sumu <sumughan@amazon.com>

gitpod-io · 2023-05-02T19:03:59Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

Signed-off-by: Sumu <sumughan@amazon.com>

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

Signed-off-by: Sumu <sumughan@amazon.com>

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

comcalvi

just a rename and this is good to go

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts

kaizencc

Do we have any information on the min/max value that is allowed? Can we create some synth time checks for them?

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

…n.test.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Signed-off-by: Sumu <sumughan@amazon.com>

kaizencc

It's looking better!

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

kaizencc · 2023-05-08T20:00:27Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

+          if (props.ephemeralStorageSize!.toGibibytes() > 200) {
+            return ['Ephemeral Storage must be at most 200 GiB.'];
+          } else if (props.ephemeralStorageSize!.toGibibytes() < 21) {
+            return ['Ephemeral Storage must be minimum 21 GiB.'];


Take a look at how we structure our other error messages and see if you can come up with the best version here.

Hello @sumupitchayan

Do you need some help to complete the PR ?
Looking at the review, this is the only comment that was not fixed.

Looking at ComputeEnvironments here an example of message could be:

ECS Fargate container '${id}' has 'ephemeralStorage' = ${props.ephemeralStorageSize} > 200 GiB; 'ephemeralStorage' cannot be greater than 200 GiB

Sorry in advance for direct ping, but it is about a week without any change and we're looking actively on this fix as it would avoid us stepping back from alpha package ...

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts

…csContainerDefinitionProps only Signed-off-by: Sumu <sumughan@amazon.com>

Signed-off-by: Sumu <sumughan@amazon.com>

… of node.addValidation Signed-off-by: Sumu <sumughan@amazon.com>

Signed-off-by: Sumu <sumughan@amazon.com>

comcalvi

Looks pretty good, some minor comments.

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

Signed-off-by: Sumu <sumughan@amazon.com>

comcalvi

minor formatting, but this is looking good!

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Signed-off-by: Sumu <sumughan@amazon.com>

comcalvi

lgtm! Nice work on this fix 🙂

kaizencc

I have a few minor comments too 🫣

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts

Signed-off-by: Sumu <sumughan@amazon.com>

kaizencc

Yay!

mergify · 2023-06-20T19:14:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-06-20T19:41:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 97fded2
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-06-20T19:42:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

add ephemeralStorage property and unit test

fe9d5ce

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation requested a review from a team May 2, 2023 19:03

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels May 2, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label May 2, 2023

aws-cdk-automation previously requested changes May 2, 2023

View reviewed changes

sumupitchayan and others added 7 commits May 2, 2023 15:13

add ephemeralStorage in README

3854e10

Signed-off-by: Sumu <sumughan@amazon.com>

add new prop to integ test

81efde2

Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

0ec9174

add new prop to IEcsContainerDefinition

2af1f44

Signed-off-by: Sumu <sumughan@amazon.com>

move ephemeral storage to Fargate class

22e06ab

Signed-off-by: Sumu <sumughan@amazon.com>

add ephemeralStorage to CloudFormation and fix test format

5d51fb2

Signed-off-by: Sumu <sumughan@amazon.com>

update integ test snapshots

3d07bd1

Signed-off-by: Sumu <sumughan@amazon.com>

remove README typo

1699e71

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation previously requested changes May 4, 2023

View reviewed changes

remove test comments

c023e0e

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan added the pr-linter/exempt-readme The PR linter will not require README changes label May 4, 2023

sumupitchayan added 2 commits May 4, 2023 15:03

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

b71a742

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

f95a4c8

sumupitchayan marked this pull request as ready for review May 5, 2023 16:47

sumupitchayan requested a review from comcalvi May 5, 2023 16:47

comcalvi suggested changes May 5, 2023

View reviewed changes

kaizencc requested changes May 5, 2023

View reviewed changes

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts Outdated Show resolved Hide resolved

kaizencc changed the title ~~feat(batch-alpha): add missing ephemeralStorage property~~ feat(batch): ephemeralStorage property on job definitions May 5, 2023

sumupitchayan and others added 2 commits May 5, 2023 16:48

Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

84c5040

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

2719d0c

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

sumupitchayan and others added 5 commits May 5, 2023 16:48

Update packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definitio…

cdc7006

…n.test.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

rename to ephemeralStorageSize

bdfb89a

Signed-off-by: Sumu <sumughan@amazon.com>

finish renaming to ephemeralStorageSize

03b220c

Signed-off-by: Sumu <sumughan@amazon.com>

add validation and unit test

8218d4c

Signed-off-by: Sumu <sumughan@amazon.com>

make prop undefined by default

58b2ce5

Signed-off-by: Sumu <sumughan@amazon.com>

kaizencc requested changes May 8, 2023

View reviewed changes

sumupitchayan and others added 6 commits May 9, 2023 11:27

refactor code for breaking change, moving executionRole property to E…

c8023c3

…csContainerDefinitionProps only Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

c2e5e1d

Kaizen suggestion - writing 20 GiB for default

d341a1f

Signed-off-by: Sumu <sumughan@amazon.com>

kaizen suggestion: simplify test case and use throw error('') instead…

06e4dab

… of node.addValidation Signed-off-by: Sumu <sumughan@amazon.com>

add test adding ephemeralStorage as a Token

d36b2b3

Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

06529b6

sumupitchayan requested a review from kaizencc May 17, 2023 16:21

comcalvi suggested changes Jun 7, 2023

View reviewed changes

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts Outdated Show resolved Hide resolved

fix calvin formatting suggestions

72ace6e

Signed-off-by: Sumu <sumughan@amazon.com>

comcalvi suggested changes Jun 8, 2023

View reviewed changes

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts Outdated Show resolved Hide resolved

sumupitchayan and others added 3 commits June 8, 2023 14:57

Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

62728cd

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Update packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

b0c672b

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

fix build failure by adding quotes within error expected error msg

ef9727d

Signed-off-by: Sumu <sumughan@amazon.com>

comcalvi approved these changes Jun 20, 2023

View reviewed changes

kaizencc requested changes Jun 20, 2023

View reviewed changes

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts Show resolved Hide resolved

packages/@aws-cdk/aws-batch-alpha/test/ecs-container-definition.test.ts Outdated Show resolved Hide resolved

kaizen suggestion, remove creating new app in unit test

b025fbd

Signed-off-by: Sumu <sumughan@amazon.com>

kaizencc approved these changes Jun 20, 2023

View reviewed changes

Merge branch 'main' into sumughan/batch-add-ephemeralstorage-prop

97fded2

mergify bot merged commit a8768f4 into main Jun 20, 2023

mergify bot deleted the sumughan/batch-add-ephemeralstorage-prop branch June 20, 2023 19:41

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Conversation

sumupitchayan commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented May 2, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaizencc May 8, 2023

Choose a reason for hiding this comment

Uh oh!

gael-ft May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jun 20, 2023

Uh oh!

aws-cdk-automation commented Jun 20, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sumupitchayan commented May 2, 2023 •

edited

Loading

gael-ft May 17, 2023 •

edited

Loading