feat(events): API Destinations#13729
feat(events): API Destinations#13729mergify[bot] merged 35 commits intoaws:masterfrom msimpsonnz:msimpson/feat-evb-api-dest
Conversation
|
|
|
@rix0rrr It would be great to get a first review on this make sure I’m in the right path |
DaWyz
left a comment
There was a problem hiding this comment.
Nice job @msimpsonnz . I added a few comments on your PR. I'm no aws-cdk maintainer so feel free to act on them if they make sense to you or ask confirmation to @rix0rrr if needed.
SvenBayer
left a comment
There was a problem hiding this comment.
@msimpsonnz, your pull request is pretty cool. I tried your code but could not get it working to add an Api Destination to my EventBridge Rule. However, I realised that the CloudWatchLogGroup is an IRuleTarget that wraps LogGroup that contains all the information. I would suggest to use here the same pattern and to wrap ApiDestination in aws-events with ApiDestination in aws-event-targets.
After my experiment I got it working. You can see my suggested changes in my comments. I hope they are helpful.
| input: this.props.event, | ||
| targetResource: this.connection, | ||
| httpParameters, | ||
| }; |
There was a problem hiding this comment.
Since we add the role as required property, we should assign it here and return it in this bind method.
| }; | ||
|
|
||
| return { | ||
| arn: this.connection.connectionArn, |
There was a problem hiding this comment.
Since we add the TargetBaseProps, we should return ...bindBaseTargetConfig(this.props), as in CloudWatchLogGroup
rix0rrr
left a comment
There was a problem hiding this comment.
Thanks for the submission!
There are a couple of small things that I'd like to see addressed (most of them already pointed out by @DaWyz), and I guess I'm not too fond of the name "API Destination" because it seems it could have been a lot more descriptive (how about HttpDestination?) but I guess we're bound to whatever EventBridge chose as their terminology...
| * | ||
| * @default none | ||
| */ | ||
| readonly headerParameters?: { [key: string]: string }; |
There was a problem hiding this comment.
Why are these here? Aren't they API gateway specific?
Doesn't it make more sense to separate this out into API Gateway destinations and "other" destinations?
There was a problem hiding this comment.
HttpParameters (headers/body/querystrings) on EventBridge targets are re-used for both ApiGateway and ApiDestinations targets
packages/@aws-cdk/aws-events-targets/test/api-destination/api-destination.test.ts
Outdated
Show resolved
Hide resolved
…/feat-evb-api-dest
|
|
Thanks @rix0rrr, I'm happy to rename but this is what EventBridge had chosen, I wasn't sure if changing this might affect functionality down the road. |
|
Thanks @DaWyz and @SvenBayer |
|
Thanks @msimpsonnz for that PR, it's really more convenient than dealing with the Cfn constructs ! Do we have an ETA for releasing it ? |
| /** | ||
| * The ARN of the connection to use for the API destination | ||
| */ | ||
| readonly connection: IConnection; |
There was a problem hiding this comment.
@rix0rrr I didn't find the documentation stating connection is optional. But in the AWS UI it's clearly required.
| apiDestinationName: 'apiDestinationName', | ||
| connection: connection, | ||
| invocationEndpoint: 'https://endpoint.com', | ||
| invocationRateLimit: Duration.seconds(10), |
There was a problem hiding this comment.
@msimpsonnz I'm confuse because you defined invocationRateLimit as number but you are passing Duration
/**
* The maximum number of requests per second to send to the HTTP invocation endpoint.
*
* @default - None
*/
readonly invocationRateLimit?: number
Is it a mistake ?
| test.done(); | ||
| }, | ||
|
|
||
| // 'create connection with InvocationHttpParameters'(test: Test) { |
924c117 to
ebfd5f2
Compare
|
Hey @msimpsonnz are you still interested in finishing this? I can provide a round of code review... |
|
@nikp yeah I am keen to continue, just wanted a review on path forward as felt there is a bit to do with Cfn generators |
|
Hope you're okay with me getting this over the finish line @msimpsonnz. Thanks for all the work so far! |
Pull request has been modified.
Pull request has been modified.
…s-cdk into pr/msimpsonnz/13729
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Adds `Connection` and `ApiDestination` constructs in Events, and an `ApiDestination` target in Events Targets. Can be used to do arbitrary HTTP calls. Fixes aws#13729 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds
ConnectionandApiDestinationconstructs in Events, and anApiDestinationtarget in Events Targets. Can be used to do arbitrary HTTP calls.Fixes #13729
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license