-
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(pipes-targets): add EventBridge #30654
Conversation
5cf3b4f
to
1bcc300
Compare
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 file names should also be camel cased, s/eventBridge/event-bridge/
@nmussy Updated. Great feedback as always. Thanks! |
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. |
06f4d5d
to
6eaf6c5
Compare
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 @msambol sorry for the long delay, have a few comment before my stamp of approval!
export class EventBridgeTarget implements ITarget { | ||
private eventBus: IEventBus; | ||
private eventBridgeParameters?: EventBridgeTargetParameters; | ||
public readonly targetArn: 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.
public readonly targetArn: string; | |
public readonly targetArn: 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.
in addition to the spacing since this is a public prop it should have a docstring... shouldn't the linter have picked up on that?
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.
@kaizencc I think because it's here ? https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-pipes-alpha/lib/target.ts#L19-L22. not entirely sure.
de40e78
to
d7912d1
Compare
Pull request has been modified.
@kaizencc – Update per your feedback. Thanks for the review! |
635d50b
to
f4575d5
Compare
@kaizencc mind giving this another pass? |
fd16301
to
32a16e1
Compare
@kaizencc I updated the integration test to incorporate feedback by @moelasmar on #30756. |
fe76476
to
f7d14ef
Compare
@Leo10Gama Could we get this one in next? |
Co-authored-by: Jimmy Gaussen <[email protected]>
f7d14ef
to
a4de2cb
Compare
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 for the contribution! Aside from a few minor typos, it looks pretty good to me
packages/@aws-cdk/aws-pipes-targets-alpha/test/event-bridge.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-pipes-targets-alpha/test/event-bridge.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Leonardo Gama <[email protected]>
Co-authored-by: Leonardo Gama <[email protected]>
…st.ts Co-authored-by: Leonardo Gama <[email protected]>
…st.ts Co-authored-by: Leonardo Gama <[email protected]>
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.
LGTM, thanks again!
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). |
Comments on closed issues and PRs are hard for our team to see. |
Add EventBridge event bus as a Pipes target.