Conversation
nickheyer
left a comment
nickheyer
left a comment
There was a problem hiding this comment.
Just a couple comments and one discussion topic, we are nearly there! Sorry again for the wait, I promise ill follow up quickly.
| } | ||
|
|
||
| // OnServerHealthy triggers MODULE_EVENT_TYPE_SERVER_HEALTHY for all modules | ||
| func (d *EventDispatcher) OnServerHealthy(ctx context.Context, serverID string) { |
There was a problem hiding this comment.
It looks like you may have removed server event dispatching for OnServerHealthy, OnPlayerJoin, and OnPlayerLeave.
There was a problem hiding this comment.
Oh, yeah my bad. It went off while I was cleaning some of my own stuff, I will revert that one.
| <ServerTasks {server} active={activeTab === 'tasks'} /> | ||
| </TabsContent> | ||
|
|
||
| <TabsContent value="webhooks" class="h-full overflow-y-auto"> |
There was a problem hiding this comment.
Discussion started in discord, but just to review - it might be a good idea to merge tasks and webhooks, to avoid redundancy in functionality as they both operate similarly.
The easy implementation would be:
A Tasks tab, and in that tab's contents there would be the existing Scheduled Tasks table, as well as your Webhooks table.
Alternatively, a more difficult, but cleaner implementation could be:
A Tasks tab, but there would be a single tasks table.
Inside that table would be both Scheduled Tasks as well as Triggered Tasks (or Event Tasks or whatever we call it). This would mean Webhook becomes a task type, and instead of tasks being purely time based scheduling, they can also hook into events!
|
|
||
| // UpdateWebhook updates an existing webhook | ||
| func (s *WebhookService) UpdateWebhook(ctx context.Context, req *connect.Request[v1.UpdateWebhookRequest]) (*connect.Response[v1.UpdateWebhookResponse], error) { | ||
| webhook, err := s.store.GetWebhook(ctx, req.Msg.Id) |
There was a problem hiding this comment.
The following is more so a note to self, not part of your review:
So there might be a bug here, though it has nothing to do with your implementation, this one is on me. in connectrpc, "empty" means null or "zeroed" value. For update routes like this, its a bit tricky if you want to "unset" or clear an existing value - given that the pattern below and elsewhere is if req.Msg.FieldVal != nil { model.Field = req.Msg.FieldVal }
If your Secret is set in CreateWebhook, but you wanted to actually clear the secret, you'd set it to an empty string input, but that would translate to nil in this update fn.
| } | ||
|
|
||
| // dispatchWebhook sends a webhook event | ||
| func (m *Manager) dispatchWebhook(ctx context.Context, serverID string, eventType storage.WebhookEventType, data map[string]interface{}) { |
There was a problem hiding this comment.
Nit, but please change map[string]interface{} to map[string]any. All interface{} should be changed to any.
2 participants
This PR adds webhooks which can be configured per server. It is possible to create generic or discord format ones.
Some looks