Skip to content
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: update all 'onXxx' methods to be CloudWatch Events #2609

Merged
merged 6 commits into from
May 23, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented May 22, 2019

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

  • All onXxx() CloudWatch Event methods now have the signature:
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.

Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

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`.
@jogold
Copy link
Contributor

jogold commented May 23, 2019

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

@rix0rrr
Copy link
Contributor Author

rix0rrr commented May 23, 2019

CW events (old pattern/signature) were added in #2326 (now merged).

Ah of course. Thanks for the heads up. I think there are some more conflicting changes on master in the linter as well.

@jogold
Copy link
Contributor

jogold commented May 23, 2019

@rix0rrr rix0rrr merged commit 28942d2 into master May 23, 2019
@rix0rrr rix0rrr deleted the huijbers/events-on-methods branch May 23, 2019 14:58
*
* @see https://docs.aws.amazon.com/codebuild/latest/userguide/sample-build-notifications.html
*/
public onEvent(id: string, options: events.OnEventOptions): events.Rule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was bivaricating on that. I found the API very ugly that way.

Hmm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.ctx.eventMethods might be more elegant

@@ -0,0 +1,71 @@
import reflect = require('jsii-reflect');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be called events.ts since it's not just CWE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert all "on" methods that are not CloudWatch events to integrations awslint:events
5 participants