go-chat-bot / bot

IRC, Slack, Telegram and RocketChat bot written in go
MIT License
826 stars 195 forks source link

how to register a package for transmitting the output of a webhook #97

Closed bnfinet closed 5 years ago

bnfinet commented 5 years ago

I'd like to implement receiving an incoming webhook (perhaps using https://github.com/go-playground/webhooks) and then notifying a channel of the event. I'm uncertain if this is the same as the #37 discussion.

Would it work to create a Webhooks.Run(&Config{...}) for use in main.go?

bnfinet commented 5 years ago

see PR #99

sochotnicky commented 5 years ago

You could make a plugin that would set up webhook receiver which would populate internal queue and periodic command that would check internal queue and possibly post message?

fabioxgn commented 5 years ago

@sochotnicky I think that the way to go is to create a new kind of command that would return a struct with a message chan and a done chan, then you would write on this chan upon receiving a message on the webhook, something like the CmdV3 works, but with 2 chans instead of just one for done: https://github.com/go-chat-bot/bot/blob/master/cmd.go#L96

This way the bot could start a goroutine that would pool this chan waiting for new messages from the webhook.

Something like:

type WebhookCmdResult struct {  
  Data chan struct {
    Message string
    Channel string
  }
  Done    chan bool
}
sochotnicky commented 5 years ago

Yeah that approach would certainly be cleaner overall. My proposal was more about a way to do it without changing the bot, but direct bot support for this approach would be nicer. I wouldn't call it "WebhookCmdResult" - maybe something like MessageStreamCmdResult with the register method called RegisterMessageStreamCommand? Or something of the sort that's not directly tied to "webhook" since I assume it could be used in different contexts?

bnfinet commented 5 years ago

Thanks for considering the issue and the technicalities.

If I want to notify irc://freenode.net/#go-bots but not any of the other platforms including irc://ourprivate.com/#inhouse? Is there a spot where the adapter/bot exposes a struct which includes network name?

Or how about reacting to irc://freenode.net by notifying a channel on irc://our private.com/#inhouse

Somewhat of an aside but related, my next itch to scratch is persistent storage which rubs up against a number of these same issues.

bnfinet commented 5 years ago

@sochotnicky @fabioxgn would love to get your feedback on #101 - thanks much!

fabioxgn commented 5 years ago

Hi @bnfinet I think I'll have some time this weekend to take a look at it, thanks for the contribution.

bnfinet commented 5 years ago

@sochotnicky @fabioxgn Happy New Year!

Do you have any time that you could spare to reviewing my PR? I have a few other features which I'd be happy to submit PRs for but it doesn't make sense to me to submit those until #101 is cleared.

Thanks much!

fabioxgn commented 5 years ago

@bnfinet sorry for the delay, I was hoping to test your changes locally for a more detailed review, but I don't think I'll have the spare time for that soon. For now I've reviewed your PR and just pointed out some documentation and forgotten comments issues, can you please fix those? Then I'll merge your PR. Thanks.

bnfinet commented 5 years ago

Thanks much @fabioxgn for the review and feedback. I've incorporated those changes you've suggested with the exception of the defer nsMap.Unlock() which when implemented causes the tests to fail. Not quite sure why that's the case but I'm going to hold off on that for now.

Do let me know if there's anything else.

fabioxgn commented 5 years ago

@bnfinet thanks, I've merged you PR.