Add riskiness label when creating rollups #138
Conversation
| if (value > 0 && value <= 0.5) { | ||
| riskiness = 'acceptably risky'; | ||
| } else if (value <= 1.0) { | ||
| riskiness = 'somewhat risky'; | ||
| } else if (value <= 2.0) { | ||
| riskinessClass = 'riskiness_medium'; | ||
| riskiness = 'risky'; | ||
| } else if (value > 2.0) { | ||
| riskinessClass = 'riskiness_high'; | ||
| riskiness = '⚠️very risky⚠️'; | ||
| } |
There was a problem hiding this comment.
These limits are super low, and I'm worried that since they'll trigger on almost every PR people will just ignore them. Here's some rollups that landed in the last few days:
- Rollup of 5 pull requests rust#84005: 5 PRs, riskiness (1 + 0 + 1 + 0 + 1 = 3) = "very risky", CI failed
- Rollup of 5 pull requests rust#83986: 5 PRs, riskiness (1 + 1 + 1 + 0 + 0 = 3) = "very risky", CI passed
- Rollup of 8 pull requests rust#83964: 8 PRs, riskiness (0 + 1 + 1 + 1 + 1 + 0 + 0 + 1 = 5) = "very risky", CI passed
None of these PRs are anything less than "very risky"; I doubt you'll find any that are in the past few weeks (there's a list at https://github.com/rust-lang/rust/pulls?q=is%3Apr+label%3Arollup; finding the riskiness is manual).
Maybe as an alternative you could raise the limit to something like 4, and mark rollup=maybe as a value of 3 instead? That seems closer to how rollups are done now.
There was a problem hiding this comment.
Honestly even the rollups I linked don't seem super risky to me, I would mark the 8 PR rollup as "riskiness_medium" and the rest as "riskiness_low". "riskiness_high" would be rollups with over 12 PRs (and I have some monsters with over 20 I could link too).
| var riskiness = 'not risky'; | ||
| var riskinessClass = 'riskiness_low'; | ||
| if (value > 0 && value <= 0.5) { | ||
| riskiness = 'acceptably risky'; |
There was a problem hiding this comment.
In practice no one will ever make a rollup with exactly 0 risk, I think you should just use "not risky" or "low risk" for both these cases. "acceptably risky" seems to imply the others are unnacceptably risky which I don't think is the right message.
This change adds a riskiness label when creating rollups that uses the number of PRs and their rollup statuses to estimate riskiness. The current formula being used is
(riskinessPoints + max(numPrs - 8, 0)) / 8where riskinessPoints is the sum of all PRs whererollup=iffyequals 2 points,rollup=maybeequals 1 point androllup=alwaysequals 0 points.Possible future improvements include: