-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(scheduler-targets): eventBridge putEvents target #27629
Conversation
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.
Hi @WtfJoke! Thanks for another scheduler target! A few comments here and there, but I appreciate your dedication to this module :)
schedule: ScheduleExpression.rate(Duration.hours(1)), | ||
target: new targets.EventBridgePutEvents(eventEntry, {}), | ||
}); | ||
``` |
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.
theres a section at the top of this readme that mentions what targets are available, can you put this and the stepfunctions one there please
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.
StepFunction was already there. I've added eventbridge. Thanks for pointing this out
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
Hey @kaizencc Yeah I initially needed StepFunction for a Demo/Talk, so I was keen in contributing this back (after hacking it together on my demo repo) and figured its quiet straight forward to add more targets (Im thinking about adding the Universal target as well, but I could imagine this will be a bit harder) :D |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3 similar comments
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
@kaizencc I've fixed the merge conflicts and rebased my changes. Please have a look at it. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
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.
Hi @WtfJoke, thanks for your continued patience here. I have a few more comments, but this is looking good!
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Show resolved
Hide resolved
* For example, events by CloudTrail have detail type "AWS API Call via CloudTrail" | ||
* @see https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-events.html | ||
*/ | ||
readonly detailType: string; |
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.
I think we can type this better than a string
. From the docs you've provided, the only thing that I see as a possible input here is "AWS API Call via CloudTrail"
. Lol. So here is what I propose:
readonly detailType: DetailType;
////
export class DetailType {
public static API_CALL_VIA_CLOUDTRAIL() {
return new DetailType('AWS API Call via CloudTrail');
}
public constructor (public readonly type: string) {}
}
This way, we can
a) give users an easy API to hit rather than getting the specific string correct (DetailType.API_CALL_VIA_CLOUDTRAIL
),
b) provide space for future additions if we know of other valid detail types as static methods,
c) users can still specify whatever they want via new DetailType('my special type')
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.
Will do.
There seems to be more, but not documented which detail-type
they emit.
I found only one additional example in the docs here "detail-type": "EC2 Instance State-change Notification"
Cant speak for others but our team uses only own detail types (e.g your point c).
For context:
I followed the existing EventBridgePutEvents implementation of the step function tasks package, which uses string as type (most likekly the type is identical).
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.
Introduced class DetailType
as per request :)
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.
Andddd I am now wavering on this suggestion. I still think it could be a good idea, but might be overengineered. I didn't see the nuance at first that most detail-types
are user-owned, and then for events from AWS there maybe detail types with specific strings.
I think we should go back to using string
here.
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.
I just dropped my last commit, which effectively reverts the change.
packages/@aws-cdk/aws-scheduler-targets-alpha/lib/event-bridge-put-events.ts
Show resolved
Hide resolved
Pull request has been modified.
@kaizencc Great to hear. Looking forward to getting this merged. Would be glad if you can answer my questions, as im stuck in 2/3 review comments. |
@kaizencc I just dropped my last commit. Also rebased my changes on to latest main and fixed all the conflicts. Please have a look :) |
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Thanks @WtfJoke and sorry for the many rounds of fruitless reviews :)
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
No worries :) |
An eventBridgePutEvents target was implemented similar to the already existing LambdaInvoke/StepFunctionStartExecution target. I needed to change some properties and methods from Target.ts from private to protected so that the logic could be reused (hope that is ok). Some design choices to outline (let me know if you disagree or have improvements I could take :) ): 1. PutEvents would accept multiple Entries (eg. an array), but I decided to support just one single event, because how Target is currently designed (to support only one target arn). It also aligns with the templated integration in the aws management console. 2. It throws an error in the constructor if the base prop `input` is used. All the props should be delivered by the new `EventBridgePutEventsEntry`. It felt not right for the developer experience to split the object (detail to `input` and `source`, `detailType` to `EventBridgePutEventsEntry` ). Closes aws#27454. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
An eventBridgePutEvents target was implemented similar to the already existing LambdaInvoke/StepFunctionStartExecution target.
I needed to change some properties and methods from Target.ts from private to protected so that the logic could be reused (hope that is ok).
Some design choices to outline (let me know if you disagree or have improvements I could take :) ):
input
is used. All the props should be delivered by the newEventBridgePutEventsEntry
. It felt not right for the developer experience to split the object (detail toinput
andsource
,detailType
toEventBridgePutEventsEntry
).Closes #27454.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license