-
Notifications
You must be signed in to change notification settings - Fork 4k
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(cloudwatch): add support for cross-account alarms #16007
Merged
Merged
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
340f1df
pipe account to metricdataquery if necessary
kaizencc 2265b01
Merge branch 'aws:master' into xa-alarms
kaizencc 0821355
fix accidental imports
kaizencc d128fcc
satisfy linter
kaizencc 317b7c1
Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …
kaizencc 749fd0a
update readme for cross-account alarms
kaizencc ed1fd3a
add renderAsLegacyMetric
kaizencc f178c89
Merge branch 'master' into xa-alarms
kaizencc f1451ff
add new test and more readme docs
kaizencc d34db67
Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …
kaizencc a7666f1
remove 1 line
kaizencc e82b5b1
Merge branch 'master' into xa-alarms
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you explain what you mean by the term "legacy" here? This doesn't quite make sense to me, since the accountId is only set if this is true, which would imply that a cross-account alarm only works if the metric is in a "legacy" format of some kind. But that doesn't make sense, since cross-account alarms are a new feature.
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 "legacy" term refers to the fact that cloudwatch has two ways to provision metrics on alarms, and only one way is being updated with new features like cross-account alarms. We want to avoid causing unnecessary diffs, so we shepherd metrics that do not require these "new" features to the "legacy" properties - the new way to provision metrics is via the
CfnAlarm.MetricDataQueryProperty
.Here
accountId
is only set ifcanRenderAsLegacyMetric
isfalse
, nottrue
. So it is correctly named here. (what seems to be the confusion is that github is hiding the unchanged lines and makes it seem likeaccountId
is inside theif (canRenderAsLegacyMetric)
scope when it is actually not.)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.
aha, thank you for the explanation! The github diff was confusing me here as well :)