Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starring can be abused #3

Open
maxpowa opened this issue Apr 5, 2016 · 5 comments
Open

Starring can be abused #3

maxpowa opened this issue Apr 5, 2016 · 5 comments
Assignees
Milestone

Comments

@maxpowa
Copy link
Contributor

maxpowa commented Apr 5, 2016

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

@dgw dgw self-assigned this Apr 19, 2019
@dgw
Copy link
Member

dgw commented Apr 19, 2019

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.

@dgw dgw added this to the 0.2.0 milestone Apr 19, 2019
@HumorBaby
Copy link
Contributor

HumorBaby commented Apr 19, 2019

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

dgw commented Apr 22, 2019

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

HumorBaby commented Apr 22, 2019

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 😁) for some problematic pushes (e.g., line notes, multiple commits, ...) so I can use them for testing?

😡 wrong place, sorry!

@dgw dgw modified the milestones: 0.2.0, 0.3.0 Aug 8, 2019
@dgw dgw modified the milestones: 0.3.0, 0.4.0 Jan 30, 2020
@dgw dgw modified the milestones: 0.4.0, 0.5.0 Jun 27, 2020
@dgw
Copy link
Member

dgw commented Jun 27, 2020

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

@dgw dgw modified the milestones: 0.5.0, 0.6.0 Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants