nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

PRChecker: support new label fast-track #104

Closed priyank-p closed 6 years ago

priyank-p commented 6 years ago

Now core has the label fast-track and a PR that uses that label so i think it is time to add support for new label that @joyeecheung opened a issue for.

codecov[bot] commented 6 years ago

Codecov Report

Merging #104 into master will increase coverage by 0.17%. The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/pr_checker.js 97.14% <100%> (+0.19%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b22d056...41cc603. Read the comment docs.

refack commented 6 years ago

Optional: still emit a warning that it's a fast-tracked PR and the lander should double check, and use discretion.

priyank-p commented 6 years ago

@refack i think that was a good suggestion to add warn msg for fast-track commits. done

joyeecheung commented 6 years ago

Hmmm, do we need to warn about that tho? Maybe info?

joyeecheung commented 6 years ago

(This reminds me I forgot to open the PR to update collaborator's guide!....)

priyank-p commented 6 years ago

@joyeecheung fixed it.

priyank-p commented 6 years ago

Adding do-not-land label until the PR is ready to get merged.

(I though the label was ready to go in core. But it seems like it not yet!)

joyeecheung commented 6 years ago

BTW: https://github.com/nodejs/node/pull/17056 has landed, the current rule is:

When a pull request is deemed suitable to be fast-tracked, label it with fast-track. The pull request can be landed once 2 or more collaborators approve both the pull request and the fast-tracking request, and the necessary CI testing is done.

priyank-p commented 6 years ago

@joyeecheung just want to know if the code-and-learn and/or doc PRs counts as fast-tracked because we currently count them as being fast-tracked.

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 run

and so it will warn if there is CI for docs (IIRC there is no CI testing for docs or maybe its linter)

joyeecheung commented 6 years ago

@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 commented 6 years ago

@joyeecheung done.