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.

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

fix(ecs): Alarms with custom dimensions should be processed #6324

Merged

Conversation

christosarvanitis
Copy link
Member

Although it is preferred/recommended to add dimensions for the Cloudwatch alarms related to the ECS Cluster Name and service name this is not enforced. In some occasions a user can create custom alarms with custom dimensions for filtering that make sense for their operations.

This PR fixes an issue where the ECS Cluster name is not part of the Cloudwatch metric and hence the id doesnt include the ECS Cluster name (because its empty).

@link108
Copy link
Member

link108 commented Jan 3, 2025

LGTM! Thank you for adding tests 🥳

Copy link
Member

@jasonmcintosh jasonmcintosh left a comment

Choose a reason for hiding this comment

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

Wish this was cleaner... but... not sure "cleaner" right now. Approving. This could use a refactor though.

@jasonmcintosh jasonmcintosh added the ready to merge Approved and ready for a merge label Jan 3, 2025
@mergify mergify bot added the auto merged Merged automatically by a bot label Jan 3, 2025
@mergify mergify bot merged commit 3e932e4 into spinnaker:master Jan 3, 2025
23 checks passed
@jasonmcintosh
Copy link
Member

@Mergifyio backport release-1.36.x

Copy link
Contributor

mergify bot commented Jan 3, 2025

backport release-1.36.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 3, 2025
mergify bot added a commit that referenced this pull request Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.37
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants