-
Notifications
You must be signed in to change notification settings - Fork 19
feat: import EventBridge events #2230
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
Conversation
e2f53e9 to
b581585
Compare
b581585 to
fbf094f
Compare
packages/@aws-cdk/service-spec-importers/src/importers/eventbridge/event-resource-matcher.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/service-spec-importers/src/importers/eventbridge/event-resource-matcher.ts
Outdated
Show resolved
Hide resolved
| eventNameSeparator?: string; | ||
| db: SpecDatabase; | ||
| }): Service | undefined { | ||
| const serviceName = getServiceName({ eventSchemaName, eventNameSeparator }); |
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.
Shouldn't serviceName be the parameter then?
General tip: if you need to parse strings (or anything, really), do the parsing as early as possible and pass structured data around inside your program.
interface EventName {
readonly serviceName: string;
readonly eventName: string;
}
function parseEventName(eventNameString: string, separator = '@'): EventName {
//
}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 changed a lot since this revision, and after the change i like more to keep this very details inside the event-resource-matcher and not leak it to the importer.
packages/@aws-cdk/service-spec-importers/src/importers/eventbridge/event-resource-matcher.ts
Outdated
Show resolved
Hide resolved
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.
Oof, so much copy pasta :(
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.
yes :(, also have a feeling some thing aren't needed but not sure. later we can improve this
91f3698 to
e6374fc
Compare
e6374fc to
f8f99e5
Compare
|
Warning Diff too large for inline display. Download the full diff using the link below. @aws-cdk/aws-service-spec: Model database diff detected |
49ffd81 to
b113d3f
Compare
|
Warning Diff too large for inline display. Download the full diff using the link below. @aws-cdk/aws-service-spec: Model database diff detected |
39802ab to
64c399e
Compare
|
Warning Diff too large for inline display. Download the full diff using the link below. @aws-cdk/aws-service-spec: Model database diff detected |
No description provided.