maxpowa commented 8 years ago

Repeatedly starring/unstarring projects monitored by sopel-github will cause spam.

dgw commented 5 years ago

Realistically, either we decide not to care about this or we remove processing of star/unstar events from the webhook handler. It's probably not an important thing to keep…

Tentatively, let's remove that handler in 0.2.0.

HumorBaby commented 5 years ago

Originally said in IRC, but I figured I'd add it here for posterity:

<HumorBaby>  I'm all for removing that for the gh-hook in here (#sopel), 
    but is it a good idea to completely remove that functionality from the main plugin? 
    I would imagine users should stop that event from being emitted from GH than the
    plugin refusing to handle it :hushed: no?

Tentatively, let's remove that handler in 0.2.0.

It could just be configurable (setting on/off, instead of being removed. Is that what you mean by "Configurable push notifications" in #19? (at least partly, since obviously there are other events to configure as well)

dgw commented 5 years ago

19 refers specifically to events of the `push` type. There are other event types as well that could use more configurability, you're right. Pull request review events come to mind (but they're tricky, because each line note comes in as a separate webhook—a limitation of GitHub's webhook system).

I think there's a fine line between keeping something and making it configurable because it's generally useful, and keeping something just because it's already in the code. Rarely do I see GitHub bots posting to IRC when the repo they monitor has been starred or unstarred—it's just not a particularly important thing to know about in a project's IRC channel.

If the plugin sets up webhooks, also, it just requests '*' (all events). Users can customize the events it gets later, but if an event type isn't useful to handle then the plugin shouldn't handle it. This is beyond making things configurable, because if the plugin supports a given event type then that code has to be maintained. We don't want to waste time maintaining code for event types that no one wants.

HumorBaby commented 5 years ago

~~I would love to help with this.~~

Just so I can try cobbling some stuff together to get the ideas flowing, would you mind sharing some of the payloads (assuming you accept my help :grin:) for some problematic pushes (e.g., line notes, multiple commits, ...) so I can use them for testing?

:rage: wrong place, sorry!

dgw commented 4 years ago

Pushing to 0.5 along with #53; several issues can all benefit from a rewrite of the webhook handling.

sopel-irc / sopel-github

Starring can be abused #3

19 refers specifically to events of the `push` type. There are other event types as well that could use more configurability, you're right. Pull request review events come to mind (but they're tricky, because each line note comes in as a separate webhook—a limitation of GitHub's webhook system).

sopel-irc / sopel-github

Starring can be abused #3

19 refers specifically to events of the push type. There are other event types as well that could use more configurability, you're right. Pull request review events come to mind (but they're tricky, because each line note comes in as a separate webhook—a limitation of GitHub's webhook system).

19 refers specifically to events of the `push` type. There are other event types as well that could use more configurability, you're right. Pull request review events come to mind (but they're tricky, because each line note comes in as a separate webhook—a limitation of GitHub's webhook system).