Skip to content

Conversation

@vito-laurenza-zocdoc
Copy link

@vito-laurenza-zocdoc vito-laurenza-zocdoc commented Dec 5, 2025

Issue #36308

Closes #36308

Reason for this change

Fixes a silent bug in the ecs module.

Description of changes

The code is mistakenly using concat() which returns a new array rather than modifying the existing one (which was the original intent of the code).

Describe any new or updated permissions being added

n/a

Description of how you validated changes

test case added

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team December 5, 2025 16:00
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 labels Dec 5, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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)

@vito-laurenza-zocdoc vito-laurenza-zocdoc changed the title Fix enable deployment alarms fix enable deployment alarms Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

TestsPassed ❌️SkippedFailed
Security Guardian Results0 ran0 passed0 skipped0 failed
TestResult
No test annotations available

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

TestsPassed ❌️SkippedFailed
Security Guardian Results with resolved templates0 ran0 passed0 skipped0 failed
TestResult
No test annotations available

@vito-laurenza-zocdoc vito-laurenza-zocdoc changed the title fix enable deployment alarms fix enableDeploymentAlarms Dec 5, 2025
@vito-laurenza-zocdoc vito-laurenza-zocdoc changed the title fix enableDeploymentAlarms fix(ecs): enableDeploymentAlarms should mutate the alarmNames property Dec 5, 2025
@pahud
Copy link
Contributor

pahud commented Dec 8, 2025

This PR is currently blocked by this:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

You will need to add/update integ test to unblock this or if you think integ tests are not necessary, please follow the suggestion #36309 (review)

@vito-laurenza-zocdoc
Copy link
Author

This PR is currently blocked by this:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

You will need to add/update integ test to unblock this or if you think integ tests are not necessary, please follow the suggestion #36309 (review)

Thanks. I've added basic coverage for this case in the existing integration test as it was not exercised there before.

@aws-cdk-automation aws-cdk-automation dismissed their stale review December 8, 2025 17:20

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(ecs): enableDeploymentAlarms does not actually add additional alarms

4 participants