doc: update CI requirements for landing pull requests #37308
Conversation
aduh95
added
meta
Unfortunately I think this will end up being very complicated. We absolutely need to run a full Jenkins CI on anything that touches a build file (e.g. any of the .gyp files including the ones in the root directory and |
doc/guides/collaborator-guide.md
Outdated
| - `deps/` | ||
| - `lib/` | ||
| - `src/` | ||
| - `test/` |
There was a problem hiding this comment.
@richardlau would this list cover everything?
| - `deps/` | |
| - `lib/` | |
| - `src/` | |
| - `test/` | |
| * `deps/` | |
| * `lib/` | |
| * `src/` | |
| * `test/` |
There was a problem hiding this comment.
No, it would miss things like (not exhaustive) tools/build-addons.js, tools/genv8constants.py, tools/js2c.py, tools/test.py, tools/v8_gypfiles
| platforms are only kept for seven days. | ||
|
|
||
| #### Useful CI jobs | ||
| A green GitHub Actions CI result is required. A passing |
There was a problem hiding this comment.
Oh, wait, GitHub Actions CI. Yeah, so maybe it is just green.
We've never required GitHub Actions CI be green before. This would be a big change in the process. I'm not opposed, necessarily, but I want to highlight this here so it doesn't escape notice.
There was a problem hiding this comment.
Since this is getting approvals, I'm going to make my concerns with this explicit. Our CI requirements are already confusing, especially to newcomers to the project (who might just expect the GitHub actions to be sufficient), and this change introduces more complexity (which I think will be unavoidable with the current mix of stuff that lives in tools/). We do not require collaborators use node-core-utils, which means our rules should aim to be simple to follow.
If we do make changes it may be simpler to state when GitHub actions only is sufficient (e.g. in addition to documentation-only changes, add changes to .github, etc.).
Even "(except it only changes comments)" is not something I'm entirely comfortable with as it's perfectly possible to break code when changing comments, e.g. mismatched /* and */ in C/C++ which could always be inside #if def's so wouldn't necessary be caught in the actions CI.
I put it there because it was already in the rules – I didn't know about it before working on this PR, and I suspect most collaborators haven't heard of it either.
Hum OK, my aim was actually to simplify the requirements by allowing more PRs to land without going to the Jenkins full run (which is sometimes flaky and causes confusion to new contributors). But I get your point, we should try to improve the collaborator experience as well.
That's something I've considered, I'm not sure it'd be ideal in the long run though: I think most files that are being added to the repo are either not related with the build process, either inside one of the listed folders; that would make the list go out-of-date quicker than the current one. Anyway, if that's the way we decide to make it, the exception list would look something like that: * `*.md`,
* `.eslintrc.js`,
* `benchmark/`,
* `doc/`,
* `tools/actions/`,
* `tools/doc/`,
* `tools/eslint-rules/`,
* `tools/node_modules/`,
* `tools/node-lint-md-cli-rollup/`,
* `tools/lint-*.*`,
* `tools/release.sh`,
* `tools/update-authors.js`
* `tools/osx-*.sh` |
richardlau
dismissed
their stale review
Clearing my block based on most recent updates in 50a2af3.
|
Adding the |
aduh95
added
author ready
PR-URL: nodejs#37308 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95
force-pushed
the
ci-requirements-pr
branch
from
50a2af3
to
fbda84e
Compare
|
Landed in fbda84e |
PR-URL: #37308 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #37308 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mary Marchini <oss@mmarchini.me> Reviewed-By: Beth Griggs <bgriggs@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The current text contain repetitions and inconsistencies. Also, I think there are some cases where Jenkins CI doesn't have any added value (linter changes, adding GH actions scripts, doc generation tool updates, etc.) but it's still required today.
This PR suggests to list folders where the full Jenkins CI is actually needed. For changes outside of those critical folders, I think GH actions results would report all the relevant mistakes already.
/cc @nodejs/tsc
If this change is accepted, I can work on a PR to reflect this change on
node-core-utils.