-
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
fix(cloudwatch): move SNS Alarm Action to aws-cloudwatch-actions
#2688
Conversation
In accordance with new guidelines, we're centralizing cross-service integrations into their own package. In this case, centralizing CloudWatch Alarm Actions into `@aws-cdk/aws-cloudwatch-actions`. Not moving AutoScaling classes because doing so would introduce circular dependencies. BREAKING CHANGE: using an SNS topic as CloudWatch Alarm Actxion now requires an integration object from the `@aws-cdk/aws-cloudwatch-actions` package.
packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-action.ts
Outdated
Show resolved
Hide resolved
* Interface for objects that can be the targets of CloudWatch alarm actions | ||
*/ | ||
export interface IAlarmAction { | ||
bind(scope: Construct, alarm: IAlarm): AlarmActionProperties; |
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 name of the return type is not super consistent across integrations, ha? Should we standardize?
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 should be Properties
. I didn't want to call it Props
because it's not a "Props" object as such, but I may have failed to rename in one PR somewhere.
Another potential candidate is Attributes
.
Your preference?
aws-cloudwatch-actions
.aws-cloudwatch-actions
I think it would make sense to use a different suffix to distinguish between those and initialization props, which have a very specific role in our framework. How about “Settings” or “Config”? |
I am also okay with [sorry just saw your previous comment asking for my preference] |
aws-cloudwatch-actions
aws-cloudwatch-actions
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.
Change suffix
Actually, we already use Attributes for the |
Gosh darnit. Just spent 1.5hrs renaming everything to "Props" :( |
In accordance with new guidelines, we're centralizing cross-service
integrations into their own package. In this case, centralizing
CloudWatch Alarm Actions into
@aws-cdk/aws-cloudwatch-actions
.Not moving AutoScaling classes because doing so would introduce circular
dependencies.
BREAKING CHANGE: using an SNS topic as CloudWatch Alarm Actxion now
requires an integration object from the
@aws-cdk/aws-cloudwatch-actions
package.
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.