balena-io-modules / balena-procbots

Process bots used for automating the development and deployment CI pipeline
https://balena-io-modules.github.io/balena-procbots/
Apache License 2.0
7 stars 3 forks source link

PokeBot: A Github user reminder system? #54

Open hedss opened 7 years ago

hedss commented 7 years ago

Given a list of repositories in a config file:

procbot:
    githubbot:
        pokebot:
            repositories:
                - resin-io/resin-image-maker
                - resin-io/resin-builder
                - ...
            timeout: 3 days

PokeBot enumerates every open PR for a repository. It sets a timer based on the timeout value and then scans all the open PRs. Should a PR not have been touched for the last timeout value, then every reviewer and the author of the PR is 'poked' by mentioning them in a comment. The timeout then resets.

We could also allow dynamic timeouts for each repo, perhaps.

One implementation detail is the limit of non-state storage for ProcBots (indeed, ProcBots should never store state). Happily, this is not an issue for PokeBot, as should it be restarted, it can scan all repos on startup and look for the last activity, then set a delta between when that occured and the timeout value to ensure it scans again at the correct interval.

lurch commented 7 years ago

So, unlike VersionBot, this would be configured just once on some global repo, rather than being configured on a per-repo basis? I guess per-repo configuration might allow more flexibility?

I wonder if it might make sense to always specify that timeout is simply an integer specifying the number of days (to make it easier to parse)? It's hard to imagine a situation where a different level of granularity would be needed?

Also, you've mixed your usages of PokeBot and ProdBot.

lurch commented 7 years ago

then set a delta between when that occured and the timeout value to ensure it scans again at the correct interval.

I guess if ProdBot is only re-scanning every timeout interval, then it'll also need to watch for changes to its configuration file, in case the timeout value gets made smaller?

hedss commented 7 years ago

So, unlike VersionBot, this would be configured just once on some global repo, rather than being configured on a per-repo basis? I guess per-repo configuration might allow more flexibility?

Yes, a global config 'somewhere'. My thoughts on dynamic timeouts for different repos does indeed map into having a value for the PokeBot in the repo config that services it and it could indeed look for a config file in the list of repos it's told to service. However, see below for an issue.

I guess if ProdBot is only re-scanning every timeout interval, then it'll also need to watch for changes to its configuration file, in case the timeout value gets made smaller?

So whilst it could be given a location for a config file to initially load (which is how I actually see this running, with a load-at-init scenario and only restarting forces a new config load), how would we signal to the PokeBot to reload configs? What's the kick? In VersionBot, the kick is a new PR event, which allows the config for that repo to be reloaded. I guess we could reload every 'period', but then if we have altered config files on a per repo basis that specify a tighter time period, this still isn't going to get reloaded until the current period expires. Maybe that's fine enough control? I guess the ProdBot could listen for commits to master for the .procbots.yml file and if it saw a change then update it. You limit the configuration to a Github repo, though.

lurch commented 7 years ago

What's the kick? ... I guess the PokeBot could listen for commits to master for the .procbots.yml file and if it saw a change then update it.

Yeah, that's pretty much what I was imagining.

You limit the configuration to a Github repo, though.

So why not have a PokeBot-per-repo, the way you have a VersionBot-per-repo? (I've not looked into how this all works, so I'm assuming my terminology is horribly incorrect) What's the reason for PokeBot having just "a global config 'somewhere' "?

hedss commented 7 years ago

Because you may not want to have any sort of config files for stuff that isn't actually ever looked at by VersionBot, such as all the process/spec. repos, but you do want people hassled if they don't comment.

lurch commented 7 years ago

Um? I was assuming that VersionBot and PokeBot are two independent systems, but from your last comment it sounds like they might not be? (i.e. it isn't possible to add just PokeBot to a repo, without adding VersionBot as well?) In that case, maybe .procbots.yml itself should specify which ProcBots do or don't operate on a particular repo? Again apologies if I'm massively misunderstanding everything again ;-)

lekkas commented 7 years ago

@lurch @hedss I guess the Prodbot term used in the comments is a Procbot typo right?

First of all, I doubt this will be a useful feature at all, because given our past experience of having Hubot pinging PR authors (reviewers were not included) this resulted in being more spam than a forcing function.

Also, If we were to use it anyway, the configuration file should be placed in the repo and leave it up to the maintainers to configure.

Because you may not want to have any sort of config files for stuff that isn't actually ever looked at by VersionBot, such as all the process/spec. repos

The recently merged PR about configuration files https://github.com/resin-io-modules/resin-procbots/pull/52 suggests that you can have many githubbot bot type and it makes sense that we should configure other bot types in the same procbot.yml conf file.

but you do want people hassled if they don't comment.

Ditto on bot spamming. Often reviewers are added to be kept in the loop of changes in the codebase and are not expected to provide reviews.

lurch commented 7 years ago

I guess the Prodbot term used in the comments is a Procbot typo right?

Nah, ProdBot was a typo for PokeBot - 'poke' and 'prod' mean similar things in english, and I guess Heds changed his mind at some point about which verb he was using ;)

Also, If we were to use it anyway, the configuration file should be placed in the repo and leave it up to the maintainers to configure.

Agreed (and that's also what I've been trying to suggest).

our past experience of having Hubot pinging PR authors (reviewers were not included) this resulted in being more spam than a forcing function.

Interesting - but hopefully @hedss 's PokeBot will be more configurable / adaptable than the Hubot script? Perhaps it suggests that there should be a procbots/pokebot/ignore type label available? Or even per-person ignore labels?

lekkas commented 7 years ago

Ah, that prod. yeah I know what prod means since Worms Armageddon :P I guessed this was a typo, a reference to 'production' or even a new bot.

hedss 's PokeBot will be more configurable / adaptable than the Hubot script

My main point here is that we should not prioritise this until we are sure that the reason for PR review bottlenecks, whenever they happen, is that people don't get enough notifications.

hedss commented 7 years ago

So I think this has spiraled out of control a bit.

To clarify! :) :

It's been suggested by a few people. I'm happy to close this issue here and now if required, but I raised it as others have spoken to me about it. :) I, personally, forget about stuff from time to time, though I make an effort to try and remember which things I need to look at. It doesn't help that Github notifications are simplistic and frankly awful, and Octobot doesn't play nicely with Firefox, which is my browser du jour.