Navigation Menu

Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

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

Closed
bnfinet opened this issue Nov 18, 2018 · 11 comments
Closed

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

bnfinet opened this issue Nov 18, 2018 · 11 comments

Comments

@bnfinet
Copy link
Contributor

bnfinet commented Nov 18, 2018

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
Copy link
Contributor Author

bnfinet commented Nov 24, 2018

see PR #99

@sochotnicky
Copy link
Contributor

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
Copy link
Member

fabioxgn commented Nov 29, 2018

@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
Copy link
Contributor

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
Copy link
Contributor Author

bnfinet commented Nov 29, 2018

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 added a commit to bnfinet/go-chat-bot that referenced this issue Dec 2, 2018
bnfinet added a commit to bnfinet/go-chat-bot that referenced this issue Dec 2, 2018
bnfinet added a commit to bnfinet/go-chat-bot that referenced this issue Dec 2, 2018
bnfinet added a commit to bnfinet/go-chat-bot that referenced this issue Dec 3, 2018
@bnfinet
Copy link
Contributor Author

bnfinet commented Dec 5, 2018

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

@fabioxgn
Copy link
Member

fabioxgn commented Dec 7, 2018

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

@bnfinet
Copy link
Contributor Author

bnfinet commented Jan 8, 2019

@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
Copy link
Member

@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 added a commit to bnfinet/go-chat-bot that referenced this issue Jan 10, 2019
@bnfinet
Copy link
Contributor Author

bnfinet commented Jan 10, 2019

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
Copy link
Member

@bnfinet thanks, I've merged you PR.

@bnfinet bnfinet closed this as completed Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants