Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(ecs-patterns): Add Ephemeral Storage Support For Ecs Patterns #25090

Closed
wants to merge 23 commits into from
Closed

feat(ecs-patterns): Add Ephemeral Storage Support For Ecs Patterns #25090

wants to merge 23 commits into from

Conversation

homakk
Copy link
Contributor

@homakk homakk commented Apr 12, 2023

Fargate supports a new ephemeral storage option, to request from 20GiB to 200
GiB of task storage. Support for it was added to aws-ecs but not to
aws-ecs-patterns.

This change adds support for ephemeral storage, so the following
patterns can take advantage of it:

  • Application load balanced fargate service
  • Application multiple target groups fargate service
  • Network load balanced fargate service
  • Network multiple target groups fargate service
  • Queue processing fargate service
  • Scheduled fargate task

Remember to follow the CONTRIBUTING GUIDE and DESIGN GUIDELINES for any
code you submit.

Closes #18105.

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

Naumel and others added 12 commits March 28, 2023 21:54
@Naumel @homakk
fix: Correct SamlConsolePrincipal for non-China (aws#24277)
90d1ac0 
Closes aws#24243.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@homakk
Merge remote-tracking branch 'upstream/main'
5734dc2
@homakk
Merge remote-tracking branch 'upstream/main'
d0af953
@homakk
Merge remote-tracking branch 'upstream/main'
8c0a1c5
@homakk
Merge remote-tracking branch 'upstream/main'
21a3b7d
@homakk
Merge remote-tracking branch 'upstream/main'
b0bdeb9
@homakk
Merge remote-tracking branch 'upstream/main'
edfa290
@homakk
Merge remote-tracking branch 'upstream/main'
6d24118
@homakk
Merge remote-tracking branch 'upstream/main'
113a350
@homakk
Merge remote-tracking branch 'upstream/main'
92cda6b
@homakk
Merge remote-tracking branch 'upstream/main'
cc35804
@homakk
feat(ecs-patterns): Add Ephemeral Storage Support For Ecs Patterns
101f3c5
@github-actions 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 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 12, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team April 12, 2023 19:08
bvtujo
bvtujo reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@bvtujo bvtujo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, shy of a few nits and a test case request

...kHistory4D520FF1.integservicecatalogproductSNSTopicProduct3B51CF591.v1.product.template.json Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ecs-patterns/lib/ecs/application-load-balanced-ecs-service.ts Outdated Show resolved Hide resolved
iamhopaul123
iamhopaul123 reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall just a very nit comment

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Outdated Show resolved Hide resolved
bvtujo
bvtujo approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@bvtujo bvtujo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

corymhall
corymhall previously requested changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts Outdated Show resolved Hide resolved
@corymhall corymhall self-assigned this Apr 19, 2023
@gitpod-io
Copy link

gitpod-io bot commented Apr 20, 2023

@mergify mergify bot dismissed corymhall’s stale review April 20, 2023 16:11

Pull request has been modified.

@homakk homakk requested a review from corymhall April 20, 2023 16:12
@mergify mergify bot dismissed corymhall’s stale review April 21, 2023 19:23

Pull request has been modified.

@homakk homakk requested a review from corymhall April 24, 2023 19:41
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

corymhall
corymhall suggested changes Apr 25, 2023
View reviewed changes
packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts
/**
* The properties for the ApplicationLoadBalancedFargateService service.
*/
export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps {
export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps, FargateTaskImageOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps, FargateTaskImageOptions {
export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps {

The ApplicationLoadBalancedFargateServiceProps should not extend
TaskImageOptions. You should add a taskImageOptions property to this
interface.

Copy link
Contributor Author

@homakk homakk Apr 25, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not working actually when I try that it throwing the following error, where taskImageOptions was already existing in
ApplicationLoadBalancedTaskImageOptions

Can you please help me out how can I make this correct.

image

image

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

3 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corymhall corymhall corymhall requested changes

@bvtujo bvtujo bvtujo approved these changes

@iamhopaul123 iamhopaul123 iamhopaul123 approved these changes

Assignees

@corymhall corymhall

Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(aws-ecs-patterns): add support for Fargate ephemeral storage
6 participants
@homakk @aws-cdk-automation @bvtujo @corymhall @iamhopaul123 @Naumel