Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Add a generic webhook for sending event notifications #879
base: main
Are you sure you want to change the base?
Feat: Add a generic webhook for sending event notifications #879
Changes from 12 commits
b15d743
20b59c3
575eb6f
e38691d
3627224
53802ac
6f14385
2d50f63
67ff8c2
780a143
2628a1a
ad0745b
c61f87c
3915a01
30cb490
7a308c7
85ac0f9
7ff4b83
bc4ed32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
What are you going to use this for ? If I remember correctly the service registry was supposed to be used to route messages to the right channel, but the channel is also passed as a parameter.
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, the idea was to use the service-registry if the service_name is supplied, but I'm not sure if everything that will use this in the future will be associated with a service. If we can make that guarantee, I can remove the channel parameter
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.
So the problem is the same as below. Please let's not have an api method with a lot of optional fields at the same level and rely on the user to know which combination are valid and which ones are not.
Instead, I would suggest one of these two so that making mistakes is harder:
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.
IT has been quite some time since when we discussed this design.
I cannot remember anymore whether the generic event was supposed to know where to route the message or whether it was supposed to be the simple api the user would use to route a message to a specific channel.
If it is the first (the user sends the message and there is logic in eng-pipes that figures out where to route the message) then it should not be a generic event and it should not contain channel specific parameters (like tags and the list of channels)
If it is the simple one (the user picks the channel), then I don't think putting all the channel fields in the same message and api is a good design, you should have dedicated apis per channel. It would be quite hard to understand which fields are supposed to be populated in which scenario.
If there are good reaons to have one api (still in the simple option where eng-pipes does not provide any routing loigic) and if you wanted to support multiple channels in one api call then a better api design would be to require a list of objects, each object containing all the details needed for a specific channel:
If there are common fields they can be specified only once.
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 the idea was to have a combination of both: a generic, simple api which a user can use to route a message to a specific channel, and a more complex webhook which has logic to route more complicated events.
I agree that the typing for the generic event payload can be improved, will work 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.
Defining the public api of a service is a task that should be thought through carefully as changing it afterwards when clients are using is hard.
So feel free to take the time to think the details of all the apis method you expect to create and send a PR with only those types and the empty business logic so we can review if the whole api is coherent and meaningful.
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.
Where are the other channels ?
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 only other one was Jira, but never really got around to getting that added since Alex was (i think?) working a little on the env setup, so I was originally waiting for him to get that done first.
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'd like a pattern where you decide which methods to call in the
genericEventNotifier
function. That is the orchestrating function, which parses the message and trigger the relevant channels. From their namesendEventToDatadog
andmessageSlack
should always send the message, not decide when not to send.Check warning on line 80 in src/webhooks/generic-notifier/generic-notifier.ts
Codecov / codecov/patch
src/webhooks/generic-notifier/generic-notifier.ts#L79-L80