git-node: add GitHub status as a CI option #369
Conversation
|
I'll add docs later, since they'll go to the same place as the docs in #367. |
|
Oh no, I thought eslint would fix those lines, but it can't autofix max-len :( I'll fix it manually and update the PR. |
mmarchini
force-pushed
the
github-ci-check
branch
from
81cfc18
to
6e48418
Compare
Codecov Report
@@ Coverage Diff @@
## master #369 +/- ##
==========================================
+ Coverage 75.98% 76.28% +0.29%
==========================================
Files 21 21
Lines 1445 1480 +35
==========================================
+ Hits 1098 1129 +31
- Misses 347 351 +4 Continue to review full report at Codecov.
|
|
Even though node uses jenkins, the jenkins results are pushed to github, so this approach might work for node as well. |
Yes, but it's less reliable since sometimes the bot won't update the status correctly. |
mmarchini
force-pushed
the
github-ci-check
branch
from
6e48418
to
fe21810
Compare
|
@joyeecheung do you mind removing the WIP tag? |
|
@mmarchini Can you rebase this branch? The Jenkins status in the Node.js repo are fairly stable now, so it would be good to use this for Node as well. Thanks! |
|
On my list. Will try to get this done today or next week. |
Some project in the org don't use Jenkins, which means PRChecker will never succeed for pull requests on those projects. These projects usually have Travis, AppVeyor or other CI systems in place, and those systems will publish the status to GitHub, which can be retrieved via API. This commit adds GitHub status as an optional way to validate if a PR satisfies the CI requirement. We need to check for the CI status in two fields returned by our GraphQL query: commit.status for services using the old GitHub integration, and commits.checkSuites for services using the new GitHub integration via GitHub Apps.
mmarchini
force-pushed
the
github-ci-check
branch
from
fe21810
to
c915920
Compare
|
Rebased |
| @@ -91,6 +92,10 @@ class Session { | |||
| return this.config.waitTimeMultiApproval; | |||
| } | |||
|
|
|||
| get ciType() { | |||
| return this.config.ciType || 'jenkins'; | |||
There was a problem hiding this comment.
I think we can make github-check the default now?
There was a problem hiding this comment.
Not yet. If we set github-check as the default, ncu will not warn users that Jenkins jobs didn't ran on a nodejs/node Pull Request, since Travis and GitHub Actions also publish to GitHub Check.
There was a problem hiding this comment.
I guess we'll need to add extra logic for Travis v.s. Jenkins status updates, then. Until then it always warns unless you do ncu-config set ciType github-check since the bot (or people) no longer posts that comment to the PRs
There was a problem hiding this comment.
since the bot (or people) no longer posts that comment to the PRs
Which comment? The bot is still commenting when a CI starts.
|
Thanks for the PR :) |
Some project in the org don't use Jenkins, which means PRChecker will
never succeed for pull requests on those projects. These projects
usually have Travis, AppVeyor or other CI systems in place, and those
systems will publish the status to GitHub, which can be retrieved via
API. This commit adds GitHub status as an optional way to validate if a
PR satisfies the CI requirement.
We need to check for the CI status in two fields returned by our GraphQL
query: commit.status for services using the old GitHub integration, and
commits.checkSuites for services using the new GitHub integration via
GitHub Apps.