feat(alarm): Support AT_LEAST when creating composite alarm#691
feat(alarm): Support AT_LEAST when creating composite alarm#691Laxenade wants to merge 3 commits intocdklabs:mainfrom
Conversation
Laxenade
changed the title
Laxenade
changed the title
lib/common/alarm/AlarmFactory.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| public abstract _renderThreshold(): string; |
There was a problem hiding this comment.
Why not a similar _bind(_operands: IAlarm[]) approach like the aws-cdk PR, which would let you avoid needing to do the checks out in determineCompositeAlarmRule instead?
There was a problem hiding this comment.
Because, unlike in _bind(), we don’t have access to the alarms in _renderThreshold(), so we can’t check the alarm count there. My initial approach was to put the common logic (i.e. 0 <= count, 0 <= percentage <= 100) in _renderThreshold() and the count-specific logic (i.e. count <= num of alarm) in determineCompositeAlarmRule, but I couldn’t convince myself having validation split across two places. In the end, I moved everything into determineCompositeAlarmRule.
There was a problem hiding this comment.
Although, on second thought, I could probably just pass the alarms into _renderThreshold() and move the validations there.
Head branch was pushed to by a user without write access
arkon
left a comment
arkon
left a comment
There was a problem hiding this comment.
LGTM, although I don't have write permissions.
|
@giantcow Mind taking a look when you have time? Thanks! |
lib/common/alarm/AlarmFactory.ts
Outdated
| super(); | ||
| } | ||
|
|
||
| public _renderThreshold(alarms: IAlarm[]): string { |
There was a problem hiding this comment.
alarms is unused, causes the build to fail
Laxenade
force-pushed
the
at-least-composite-alarm
branch
from
bbcdd48 to
4acb36f
Compare
Laxenade
force-pushed
the
at-least-composite-alarm
branch
from
4acb36f to
5d3be00
Compare
|
@giantcow Bump |
3 participants
Adding support for creating composite alarms using the new
AT_LEASToperator. The corresponding change in cdk-lib has not been merged aws/aws-cdk#36100, and historically, this package lags behind cdk-lib by several months. Therefore, instead of using the new construct in cdk-lib, we will compose the alarm string ourselves. This is not a big deal since most of the code in the PR is still necessary. The only aspect that may change once we have the construct in cdk-lib is likely the last few lines indetermineCompositeAlarmRule.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license