PRChecker: support new label fast-track #104
Conversation
Codecov Report
@@ Coverage Diff @@
## master #104 +/- ##
==========================================
+ Coverage 91.11% 91.28% +0.17%
==========================================
Files 14 14
Lines 540 551 +11
==========================================
+ Hits 492 503 +11
Misses 48 48
Continue to review full report at Codecov.
|
|
Optional: still emit a warning that it's a fast-tracked PR and the lander should double check, and use discretion. |
|
@refack i think that was a good suggestion to add warn msg for |
|
Hmmm, do we need to warn about that tho? Maybe info? |
|
(This reminds me I forgot to open the PR to update collaborator's guide!....) |
lib/pr_checker.js
Outdated
| (labels.length === 1 && labels[0].name === 'doc'); | ||
|
|
||
| if (isFastTrack) { | ||
| cli.warn('No wait time! "fast-track" labelled PR.'); |
There was a problem hiding this comment.
Maybe just: This PR is being fast-tracked, they can always look up the labels in the logIntro() messages. Also I prefer an info over warning.
lib/pr_checker.js
Outdated
| @@ -164,6 +172,11 @@ class PRChecker { | |||
| return false; | |||
| } | |||
|
|
|||
| function isFast(label) { | |||
| isFastTrack = label === 'fast-track'; | |||
There was a problem hiding this comment.
Why do we need a closure here? All the checks can be done in the function passed to labels.some()
There was a problem hiding this comment.
@joyeecheung To check afterwards if the label was fast-track and log the message out.
There was a problem hiding this comment.
@cPhost But now that we don't log that in the messages...we don't really need this right? All we need is
const fast = labels.some(l => ['code-and-learn', 'fast-track'].includes(l.name)) ||
(labels.length === 1 && labels[0].name === 'doc');
if (fast) {
cli.info(...)
return true;
}There was a problem hiding this comment.
@joyeecheung Oh i though we do not count code-and-learn fixing it right now.
priyank-p
force-pushed
the
fast-track-label
branch
from
e4e1d2c
to
c45d459
Compare
|
@joyeecheung fixed it. |
|
Adding (I though the label was ready to go in core. But it seems like it not yet!) |
priyank-p
force-pushed
the
fast-track-label
branch
from
c45d459
to
80f64b8
Compare
|
BTW: nodejs/node#17056 has landed, the current rule is:
|
priyank-p
added
Work In Progress
|
@joyeecheung just want to know if the BTW this is what i currently have: # if it can be fast-tracked, have two approvals a CI cli.info
This PR is being fast-tracked
# else if missing approvals or ci or both cli.warn
This PR is being fast-tracked, but awaiting approvals of 2 contributors # or
This PR is being fast-tracked, but awaiting a CI run # or
This PR is being fast-tracked, but awaiting approvals of 2 contributors and a CI runand so it will warn if there is CI for docs (IIRC there is no CI testing for docs or maybe its linter) |
|
@cPhost I would suggest to skip the code-and-learn and doc rules and just check for fast-track. If it should be fast-tracked, people should label it. |
priyank-p
force-pushed
the
fast-track-label
branch
from
80f64b8
to
41cc603
Compare
|
@joyeecheung done. |
Now core has the label
fast-trackand a PR that uses that label so i think it is time to add support for new label that @joyeecheung opened a issue for.