feat: including 'AWS Event Bridge' in 'Notifiers' #7
Conversation
adamzero1
commented
Aug 6, 2024
adamzero1
commented
| public function afterToOptionArray(Subject $subject, $result) | ||
| { | ||
| $result[] = [ | ||
| 'value' => 'event_bridge', |
There was a problem hiding this comment.
the key is eventbridge
ref: https://github.com/mage-os/mageos-async-events-aws/blob/main/etc/di.xml#L7
| @@ -9,4 +9,8 @@ | |||
| </argument> | |||
| </arguments> | |||
| </type> | |||
|
|
|||
| <type name="MageOS\AsyncEventsAdminUi\Ui\Source\Notifiers"> | |||
There was a problem hiding this comment.
this would mean that this module needs to depend on the MageOSAsyncEventsAdminUi module which hasn't been declared.
rather than adding a plugin here, could we make the change in the admin module to either
- accept free text input
- auto discover notifiers present in
MageOS\AsyncEvents\Service\AsyncEvent\NotifierFactory🤔
There was a problem hiding this comment.
I've created a very vague test PR mage-os/mageos-async-events#29
There was a problem hiding this comment.
cheers for the feeedback @gowrizrh ,
this would mean that this module needs to depend on the MageOSAsyncEventsAdminUi module which hasn't been declared.
It doesn't break with it in, but also didn't want expand the admin ui module (as then that would mean all sink types would need adding in there), it also felt a bit of a small plugin to warrant its own module.
What is it you need from me to get this over the line? (The project I'm using this on is almost ready, would rather take a packaged version then a fork live)
There was a problem hiding this comment.
I'm back from holidays and approved @gowrizrh's PR in the main module. We can push forward now. I'd like a change in the Admin UI module more, we can finish that now.
There was a problem hiding this comment.
@adamzero1 I'm back from holidays. I'm happy to approve this to get it across the line if updating the Admin UI module is going to be a while @avstudnitz. Once the admin module lists all event sinks automatically I'll likely revert this change 🙂
