-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(stepfunctions-tasks): allow EcsRunTask on fargate and ec2 to set capacity provider strategy #35465
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
base: main
Are you sure you want to change the base?
Conversation
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| @@ -1,3 +1,3 @@ | |||
| FROM public.ecr.aws/docker/library/python:3.12 | |||
| FROM --platform=linux/amd64 public.ecr.aws/docker/library/python:3.12 | |||
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.
Because amd64 is used for cpu architecture in integ tests.
If this option is not specified, running the tests on Mac will result in an error during state machine execution.
badmintoncryer
left a comment
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.
Thank you for your contribution! I've added some minor comments.
| stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false); | ||
| stack.node.setContext(STEPFUNCTIONS_TASKS_FIX_RUN_ECS_TASK_POLICY, false); |
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.
Are these feature flags are needed?
| postCliContext: { | ||
| '@aws-cdk/aws-ecs:enableImdsBlockingDeprecatedFeature': false, | ||
| '@aws-cdk/aws-ecs:disableEcsImdsBlocking': false, | ||
| }, |
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.
Are there feature flags needed?
| stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false); | ||
| stack.node.setContext(STEPFUNCTIONS_TASKS_FIX_RUN_ECS_TASK_POLICY, false); |
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 think it is better to unify the method for setting feature flags either through postCliContext or setContext. (If these flags are needed!)
| The `capacityProviderOptions` property allows you to configure the capacity provider | ||
| strategy for both EC2 and Fargate launch targets. | ||
|
|
||
| - When `CapacityProviderOptionsBase.none()` is used, the task uses the launch type (EC2 or FARGATE) without a capacity provider strategy. |
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.
It's true that CapacityProviderOptionsBase is an abstract base class, but I don't think users will be aware of the concrete classes that inherit from it when specifying their settings.
I thought it would be more user-friendly to name it simply CapacityProviderOptions. What do you think?
| /** | ||
| * No capacity provider strategy options | ||
| */ | ||
| export class NoCapacityProviderOptions extends CapacityProviderOptionsBase { |
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.
Should this concrete class be exported?
I think user uses not a NoCapacityProviderOptions but a abstract class.
| ], | ||
| launchTarget: new tasks.EcsFargateLaunchTarget({ | ||
| platformVersion: ecs.FargatePlatformVersion.VERSION1_4, | ||
| capacityProviderOptions: tasks.NoCapacityProviderOptions.none(), |
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.
Is it better to use a abstract class?
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.
Relates to #35465 (comment)
|
|
||||||||||||||||||||||
|
|
||||||||||||||
|
@badmintoncryer Thanks for your review! I'll check and apply your points later! |
|
I have applied the all comments! |
badmintoncryer
left a comment
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.
Thanks!!
ozelalisen
left a comment
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.
Have some comments, and thanks for raising this PR, this will be really valuable!
| } | ||
|
|
||
| interface CapacityProviderParams { | ||
| launchType?: ecs.LaunchType; |
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.
The launchType in CapacityProviderParams feels unnecessary here. The launch type is already determined by the task context (Fargate vs EC2) — it's not something users should configure via capacityProviderOptions. The current _bind(launchType) pattern passes it in just to return it back out.
Could we simplify by just checking whether capacityProviderStrategy is provided instead of threading launchType through this abstraction?
| /** | ||
| * No capacity provider strategy is used. | ||
| */ | ||
| public static none(): CapacityProviderOptions { |
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'm not sure CapacityProviderOptions.none() adds value here. The absence of the capacityProviderOptions property already means "no capacity provider strategy" — having an explicit .none() method creates two ways to express the same thing (undefined vs .none()), which can be confusing for users.
What's the use case for explicitly calling .none() vs just not setting the property?
| * @default - 'FARGATE' LaunchType running tasks on AWS Fargate On-Demand | ||
| * infrastructure is used without the capacity provider strategy. | ||
| */ | ||
| readonly capacityProviderOptions?: CapacityProviderOptions; |
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 think a simpler API without the abstract class hierarchy would be better, something like:
capacityProviderStrategy?: ecs.CapacityProviderStrategy[];
useClusterDefaultCapacityProvider?: boolean;Then the logic becomes:
- capacityProviderStrategy provided → use custom strategy (omit LaunchType)
- useClusterDefaultCapacityProvider: true → omit both (uses cluster default)
- Neither → use LaunchType (current behavior)
This would remove the need for CapacityProviderOptions, NoCapacityProviderOptions, CustomCapacityProviderOptions, and DefaultCapacityProviderOptions classes entirely, making the API more intuitive and easier to maintain.
| * @default - 'EC2' LaunchType running tasks on Amazon EC2 instances registered to | ||
| * your cluster is used without the capacity provider strategy. | ||
| */ | ||
| readonly capacityProviderOptions?: CapacityProviderOptions; |
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.
Same comment as above
Issue # (if applicable)
Closes #20013 #30171 #7967
Reason for this change
The capacity provider strategy can't be set on EcsRunTask with
EcsFargateLaunchTargetOptionsandEcsEc2LaunchTargetOptions.capacityProviderStrategy
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RunTask.html
Description of changes
This PR adds a new property
capacityProviderOptions(CapacityProviderOptionsclass with static factory methods as an union-like class) in the target options.The
CapacityProviderOptionshas following sub types:The
NoCapacityProviderOptionscreates the original settings withLaunchType(EC2orFARGATE). It is a default value for thecapacityProviderOptions.The
CustomCapacityProviderOptionsallows users to set the capacity provider strategy withoutLaunchType.The
DefaultCapacityProviderOptionsallows users to use the cluster's default capacity provider strategy. The default strategy can be set by specifying no options (LaunchTypeandCapacityProviderStrategy) in CFn.Describe any new or updated permissions being added
Description of how you validated changes
Both unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license