fix: update all 'onXxx' methods to be CloudWatch Events #2609
Conversation
Make all CloudWatch events methods consistent. All methods called 'onXxx()' are now guaranteed to be CloudWatch Events triggers, all non-CWE methods that started with 'on' are now called differently. Introduce the 'onCloudTrailEvent' convention, to clearly signal that the indicated event depend on the presence of a CloudTrail Trail to capture it (as opposed to being intrinsically supported by the service itself). Introduce awslint rules to enforce these conventions. Fixes #2278. BREAKING CHANGES * All `onXxx()` CloudWatch Event methods now have the signature: ```ts resource.onEvent('SomeId', { target: new SomeTarget(...), // options }); ``` * CloudWatch: rename `onAlarm()` => `addAlarmAction()`, `onOk` => `addOkAction`, `onInsufficientData` => `addInsufficientDataAction`. * AutoScaling: rename `onLifecycleTransition` => `addLifecycleHook()`. * LambdaDeploymentGroup: rename `onPreHook` => `addPreHook`, `onPostHook` => `addPostHook`. * UserPool: rename all `onXxx` methods => `addXxxTrigger`. * Repository: rename `onImagePushed` => `onCloudTrailImagePushed`. * Bucket: rename `onEvent` => `addEventNotification`, `onObjectCreated` => `addObjectCreatedNotification`, `onObjectRemoved` => `addObjectRemovedNotification`, `onPutObject` => `onCloudTrailPutObject`.
rix0rrr
requested review from
RomainMuller,
skinny85,
SoManyHs and
a team
as code owners
|
CW events (old pattern/signature) were added in #2326 (now merged). The README should also be updated: https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-config/README.md#events |
Ah of course. Thanks for the heads up. I think there are some more conflicting changes on master in the linter as well. |
|
@rix0rrr there's also the example in the README https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-config/README.md#events |
| * | ||
| * @see https://docs.aws.amazon.com/codebuild/latest/userguide/sample-build-notifications.html | ||
| */ | ||
| public onEvent(id: string, options: events.OnEventOptions): events.Rule { |
There was a problem hiding this comment.
I guess I missed this in the previous PR, but it's now or never: I kind of think that "target" shouldn't be under "Options", so I can do onEvent('foo', target).
It also maps more cleanly to the fact that you pass in options to events.Rule and then addTarget separately.
I would also say that events.RuleProps should contain targets (plural) to allow adding targets declaratively...
There was a problem hiding this comment.
I was bivaricating on that. I found the API very ugly that way.
Hmm.
There was a problem hiding this comment.
Actually, here's why I changed the signature: the signature with 3 arguments becomes very ugly if you have a complex target (such as an ECS task or CodeBuild project), or a complex payload (from a JSON message), and then also configuration after it.
It would look something like this:
source.onEvent('MyEvent', rule.addTarget(new targets.EcsEc2Task({
cluster,
taskDefinition,
taskCount: 1,
containerOverrides: [{
containerName: 'TheContainer',
environment: [{
name: 'I_WAS_TRIGGERED',
value: 'From CloudWatch Events'
}]
}]
}), {
eventDescription: 'hello'
eventPattern: {
field: ['value']
}
});vs:
source.onEvent('MyEvent', {
eventDescription: 'hello'
eventPattern: {
field: ['value']
},
target: new targets.EcsEc2Task({
cluster,
taskDefinition,
taskCount: 1,
containerOverrides: [{
containerName: 'TheContainer',
environment: [{
name: 'I_WAS_TRIGGERED',
value: 'From CloudWatch Events'
}]
}]
})
});I feel the bad case is a lot worse for the first API, where the 2nd API fixes that, and the regular case isn't THAT much worse.
There was a problem hiding this comment.
Also, I think obj.method('id', { ...props... }) is more consistent with most of our other APIs.
| // the scope is also the ignore pattern and they're all conceptually the same rule. | ||
| // | ||
| // Simplest solution is to not record successes -- why do we even need them? | ||
| if (include && condition) { return condition; } |
There was a problem hiding this comment.
It was mostly for verbose logging, maybe we can emit those here in case we are in verbose mode?
| } | ||
| } | ||
|
|
||
| const ON_EVENT_OPTIONS_FQN = '@aws-cdk/aws-events.OnEventOptions'; |
There was a problem hiding this comment.
Should we move these to CoreTypes, did Shiv merge this?
| code: 'events-in-interface', | ||
| message: `'onXxx()' methods should also be defined on construct interface`, | ||
| eval: e => { | ||
| for (const method of e.ctx.directEventMethods.concat(e.ctx.cloudTrailEventMethods)) { |
There was a problem hiding this comment.
e.ctx.eventMethods might be more elegant
| @@ -0,0 +1,71 @@ | |||
| import reflect = require('jsii-reflect'); | |||
There was a problem hiding this comment.
Should probably be called events.ts since it's not just CWE
Make all CloudWatch events methods consistent. All methods called
'onXxx()' are now guaranteed to be CloudWatch Events triggers, all
non-CWE methods that started with 'on' are now called differently.
Introduce the 'onCloudTrailEvent' convention, to clearly signal
that the indicated event depend on the presence of a CloudTrail
Trail to capture it (as opposed to being intrinsically supported
by the service itself).
Introduce awslint rules to enforce these conventions.
Fixes #2278, fixes #2443.
BREAKING CHANGES
onXxx()CloudWatch Event methods now have the signature:onAlarm()=>addAlarmAction(),onOk=>addOkAction,onInsufficientData=>addInsufficientDataAction.onLifecycleTransition=>addLifecycleHook().onPreHook=>addPreHook,onPostHook=>addPostHook.onXxxmethods =>addXxxTrigger.onImagePushed=>onCloudTrailImagePushed.onEvent=>addEventNotification,onObjectCreated=>
addObjectCreatedNotification,onObjectRemoved=>addObjectRemovedNotification,onPutObject=>onCloudTrailPutObject.Pull Request Checklist
designfolderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.