-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(aws-cloudwatch-actions): add ssm incidents as alarm action #21167
Conversation
…tion This small PR will add SSM Incident action to cloudwatch alarm. The arn format was taken from the UI console (under Incident Manager Response Plan)
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.
Overall this looks great! I think we'll need an integration test for this, though. Could you please add one?
+ fix typo
Pull request has been modified.
I've added an integration test using |
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.
@callumthomson Your integ test file is what we're looking for. We just need you to generate the expected template and assets. See our integration test guide.
I've also added a few comments inline regarding the specific errors in the build.
packages/@aws-cdk/aws-cloudwatch-actions/test/integ.ssm-incident-alarm-action.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-cloudwatch-actions/test/integ.ssm-incident-alarm-action.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
@TheRealAmazonKendra I've added in the requested changes. Please let me know any further desired amendments. |
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.
Just one comment inline but it also looks like your test can use SSM Incident as alarm action
isn't working because the partition is a reference in the actual output, but you've used a literal in your expected output. We just need to get this test working so the build passes. If you have any questions about this, please let me know.
Pull request has been modified.
@TheRealAmazonKendra that seems to be working now, but there appears to have been an error in
|
@Mergifyio update |
✅ Branch has been successfully updated |
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Finishes work originally contributed by @dosatos by addressing changes requested by @comcalvi in #20553
closes #20552