nodejs-github-bot example PR

[Fishrock123]

nodejs-github-bot example PR

As of nodejs/github-bot@79f828c, the nodejs-github-bot is capable of labeling pull requests based on the files within.Some additional examples can be found at https://github.com/TestOrgPleaseIgnore/node/pulls

[Fishrock123]

@nodejs/ctc

I temporarily enabled the bot here to show this. It is currently disabled.

It requires write permissions (which have been given to other bots in the past), as well as webhook notifications of PR events.

At the current time it only matches a very small subset of labels, to start. Only two, infact.

• test if the pull request contains only tests.
• c++ if the pull request touches src/

[Fishrock123]

also cc @phillipj & @williamkapke who have been working on this too.

[phillipj]

@Fishrock123 does five 🎉s on your PR description warrant enabling the bot permanently? At least for a longer periode than a few minutes..

[jbergstroem]

Other low hanging fruit might be:

• doc/ for doc
• benchmark/ for benchmark

[Fishrock123]

Discussed this with @rvagg and he was concerned that this may mean issues do not get the human eyes they need.

cc-ing some our top taggers here to see what they think, particularly @mscdex, but also @Trott, @ChALkeR, @jasnell, @thealphanerd

[jasnell]

Worth experimenting with.

[mscdex]

I think if we go ahead with it, we definitely should start with the simplest cases where it's really easy to know the correct tag(s). My concern would be having to go back and remove incorrect/misleading tags.

[jbergstroem]

Decreasing visibility makes assumptions on how people use github. I don't use tags much myself -- the best way to connect is in my opinion assigning an issue/pr, and next being mentioned. In time (when github supports assigning teams), it'd be pretty sweet to try and assign a team based on whats being changed.

[phillipj]

Worth noting the bot will only try to find correct labels when the PR is created. Subsequent changes are currently ignored, this might change in the future.

I think if we go ahead with it, we definitely should start with the simplest cases where it's really easy to know the correct tag(s). My concern would be having to go back and remove incorrect/misleading tags.

ATM we've got these two scenarios:

• c++ if some of the files changed was located in src/ including any sub dirs
• test if all the changed files was located in test/ including any sub dirs

@mscdex would you consider these safe bets?

[mscdex]

@phillipj I think the c++ tag could be tricky, but in general any non-strictly test or benchmark or similar changes could be tricky. I am not sure what the bot implementation takes into account currently, but what about the possibility of devising some kind of threshold for net lines changed or something for a particular file when determining what tag to use?

For example, there might be a PR that makes changes to both lib/net.js and src/tcp_wrap.cc, but the overall net change in lib/net.js is substantially greater than that of src/tcp_wrap.cc. In that particular case I would normally just tag it with net only.

[jbergstroem]

Moar low hanging fruit:

• deps/$name -> $name (libuv, v8, etc)
• configure, *Makefile, node.gyp, common.gypi -> build (a lot of stuff in tools/ lives here too but not necessarily low hanging fruit)

[Fishrock123]

For example, there might be a PR that makes changes to both lib/net.js and src/tcp_wrap.cc, but the overall net change in lib/net.js is substantially greater than that of src/tcp_wrap.cc. In that particular case I would normally just tag it with net only.

Usually I just tag anything that has remotely significant c++ in it with c++. :P

[Fishrock123]

Ok based on feedback I've enabled it. I'll leave this open for now unless someone thinks I should re-make it as an issue rather than a PR.

[phillipj]

I am not sure what the bot implementation takes into account currently, but what about the possibility of devising some kind of threshold for net lines changed or something for a particular file when determining what tag to use?

@mscdex currently it only looks at file paths affected. How many lines changed is not taken into consideration. Your idea seems like a good fix if we see bad labelling due to its current trivial brains.

Summarised possible improvements based on the feedback so far nodejs/github-bot#31. Feel free to add more thoughts and tips to that issue.

[phillipj]

FYI just merged and deployed more labels: doc, benchmark and deps/X which was mentioned as low hanging fruits in previous comments ^