Skip to content

fix(ecs): allow empty placementStrategies on EC2Service #35580

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
Merged
merged 5 commits into from
Nov 10, 2025
Merged

fix(ecs): allow empty placementStrategies on EC2Service #35580

merged 5 commits into from
Nov 10, 2025

Conversation

@isker
Copy link
Contributor

@isker isker commented Sep 26, 2025 •
edited
Loading

Reason for this change

You can't remove placement strategies from an ECS service using CDK once they've been set.

Description of changes

Same fix as #30382 but for placementStrategies. Apparently this was not done at that time because CloudFormation previously did not have the appropriate semantics for setting PlacementStrategies to the empty array as it did for PlacementConstraints, but that is no longer the case. The CloudFormation docs state:

To remove this property from your service resource, specify an empty PlacementStrategy array.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Unit and integration tests.

Checklist

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

@github-actions github-actions bot added the p2 label Sep 26, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team September 26, 2025 01:28
@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Sep 26, 2025
aws-cdk-automation
aws-cdk-automation previously requested changes Sep 26, 2025
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(This review is outdated)

@isker isker changed the title fix(aws-ecs): allow empty placementStrategies on EC2Service fix(ecs): allow empty placementStrategies on EC2Service Sep 26, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 26, 2025 01:37

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

@isker isker force-pushed the ecs-empty-placementstrategies branch from 415b28c to dcdd1f7 Compare September 26, 2025 03:18
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 26, 2025
@github-actions github-actions bot mentioned this pull request Oct 1, 2025
Open
@vishaalmehrishi vishaalmehrishi self-assigned this Oct 24, 2025
vishaalmehrishi
vishaalmehrishi previously requested changes Oct 24, 2025
View reviewed changes
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 24, 2025
@mergify mergify bot dismissed vishaalmehrishi’s stale review October 24, 2025 20:41

Pull request has been modified.

@isker isker requested a review from vishaalmehrishi October 24, 2025 20:42
@isker
Copy link
Contributor Author

isker commented Oct 24, 2025

Build failure just looks like an oom...

isker added 2 commits October 24, 2025 17:10
@isker
fix(aws-ecs): allow empty placementStrategies on EC2Service
c8c50ff 
Same fix as aws#30382 but for `placementStrategies`. Apparently this was
not done at that time because CloudFormation [previously did not have
the appropriate
semantics](aws#27572 (comment))
for setting PlacementStrategies to the empty array as it did for
PlacementConstraints, but that is no longer the case. The CloudFormation
docs
[state](https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-resource-ecs-service.html#cfn-ecs-service-placementstrategies):
> To remove this property from your service resource, specify an empty PlacementStrategy array.

And indeed that works, with this CDK change applied.
@isker
Code review feedback
cc0397b
@isker isker force-pushed the ecs-empty-placementstrategies branch from 0597424 to cc0397b Compare October 24, 2025 21:10
@mergify mergify bot dismissed vishaalmehrishi’s stale review November 1, 2025 03:26

Pull request has been modified.

@isker isker requested a review from vishaalmehrishi November 1, 2025 04:41
@isker
Copy link
Contributor Author

isker commented Nov 10, 2025

@vishaalmehrishi please take another look.

@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2025

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).

@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/codebuild-pr-build.yml without workflows permission.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot dismissed vishaalmehrishi’s stale review November 10, 2025 17:49

Pull request has been modified.

@isker
Copy link
Contributor Author

isker commented Nov 10, 2025

@vishaalmehrishi I merged from main, can you try to land this PR again? I don’t think I’m allowed to do it myself.

@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2025

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).

@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2025

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).

@mergify mergify bot merged commit 0d773b1 into aws:main Nov 10, 2025
19 of 20 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vishaalmehrishi vishaalmehrishi vishaalmehrishi approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees

@vishaalmehrishi vishaalmehrishi

Labels

p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@isker @vishaalmehrishi @aws-cdk-automation