Add webhooks #192
Add webhooks #192
Conversation
host/webhooks/webhooks.go
Outdated
| ) | ||
|
|
||
| type Manager struct { | ||
| ctx context.Context |
There was a problem hiding this comment.
It's bad practice to store a context on a struct. https://go.dev/blog/context-and-structs
host/webhooks/webhooks.go
Outdated
| store WebhookStore | ||
|
|
||
| mu sync.Mutex | ||
| queues map[string]*eventQueue // URL -> queue |
There was a problem hiding this comment.
This is fine, but seems overkill. A single queue would also probably be fine for
this small of a usecase.
|
Thanks for the comments! I'll get on adjusting. |
persist/sqlite/migrations.go
Outdated
| hook_url TEXT NOT NULL | ||
| );` | ||
|
|
||
| if _, err = tx.Exec(query); err != nil { |
There was a problem hiding this comment.
you can remove the extra if and context and return
_, err := tx.Exec(...)
return err
persist/sqlite/webhooks.go
Outdated
| } | ||
|
|
||
| func (s *Store) Webhooks() (whs []webhooks.Webhook, err error) { | ||
| rows, err := s.db.Query(`SELECT * FROM webhooks;`) |
There was a problem hiding this comment.
Be explicit about columns SELECT id, scope, ... FROM webhooks
persist/sqlite/webhooks.go
Outdated
| func (s *Store) AddWebhook(wb webhooks.Webhook) (err error) { | ||
| var count int | ||
|
|
||
| err = s.db.QueryRow(`SELECT COUNT(*) FROM webhooks WHERE scope = ? AND hook_url = ?;`, strings.Join(wb.Scope, ","), wb.URL).Scan(&count) |
There was a problem hiding this comment.
No need for the extra SELECT COUNT(*), just INSERT. Also add ON CONFLICT (hook_url) DO UPDATE for upserts
| } | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| m := &Manager{ | ||
| ctx: ctx, |
There was a problem hiding this comment.
Remove the context.Context and context.CancelFunc from the struct
|
I'm having a real busy week which is why i haven't had time to look further at this, but I think I'll have time this weekend. Just so you know I haven't forgotten about it. |
|
Hey @multikatt, are you still implementing this feature? |
|
@justmert @n8maninger Im sorry about leaving this for so long, I was hoping to have time to get back to it but life's gotten in the way unfortunately so I'm just not going to be able to finish this within a reasonable time frame. Hopefully somebody can use what I've done so far. |
No worries. I’ll finish it out. Thanks for getting this far |
|
Hi, sorry for impatient me. Is there a time when it will be implemented? Since I need this functionality. Thank you @n8maninger |
DRAFT!
This PR implements webhooks in hostd to address #189.
It's not done all the way though, but before going too far I want to confirm I'm on the right track. The code here is heavily "borrowed" from renterd, so there's a fair bit of duplication. Is this ok or would it be better to reuse the code from renterd? What would be the preferred method of doing it in that case?
Lacking in the version here is
the ability to delete webhooks, add actions andcleaning things up a bit.