Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: consolidate CI sections #42534

Merged
merged 2 commits into from
Apr 1, 2022
Merged

doc: consolidate CI sections #42534

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12bfec2
doc: consolidate CI sections
Trott Mar 30, 2022
2b01fc6
Update doc/contributing/pull-requests.md
Trott Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions doc/contributing/pull-requests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
* [Notes](#notes)
* [Commit squashing](#commit-squashing)
* [Getting approvals for your pull request](#getting-approvals-for-your-pull-request)
* [CI testing](#ci-testing)
* [Waiting until the pull request gets landed](#waiting-until-the-pull-request-gets-landed)
* [Check out the collaborator guide](#check-out-the-collaborator-guide)
* [Appendix: subsystems](#appendix-subsystems)
Expand Down Expand Up @@ -511,12 +510,14 @@ feedback.
All pull requests that contain changes to code must be run through
continuous integration (CI) testing at [https://ci.nodejs.org/][].

Only Node.js core collaborators with commit rights to the `nodejs/node`
repository may start a CI testing run. The specific details of how to do
this are included in the new collaborator [Onboarding guide][].
Only Node.js core collaborators and triagers can start a CI testing run. The
specific details of how to do this are included in the new collaborator
[Onboarding guide][]. Usually, a collaborator or triager will start a CI
test run for you as approvals for the pull request come in.
If not, you can ask a collaborator to start a CI run.
Trott marked this conversation as resolved.
Show resolved Hide resolved

Ideally, the code change will pass ("be green") on all platform configurations
supported by Node.js (there are over 30 platform configurations currently).
supported by Node.js. (There are over 30 platform configurations currently.)
Copy link
Contributor

@bnb bnb Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's strange to have a whole sentence as a parenthetical (and also... off to have currently at the end of that parenthetical). Two potential alternatives:

Suggested change
Ideally, the code change will pass ("be green") on all platform configurations
supported by Node.js (there are over 30 platform configurations currently).
supported by Node.js. (There are over 30 platform configurations currently.)
Ideally, the code change will pass ("be green") on all 30+ platform
configurations supported by Node.js.
Suggested change
Ideally, the code change will pass ("be green") on all platform configurations
supported by Node.js (there are over 30 platform configurations currently).
supported by Node.js. (There are over 30 platform configurations currently.)
Ideally, the code change will pass ("be green") on all platform configurations
supported by Node.js. There are currently over 30 platform configurations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with Option 3: Delete the sentence entirely because it does not contain essential information. PTAL.

This means that all tests pass and there are no linting errors. In reality,
however, it is not uncommon for the CI infrastructure itself to fail on
specific platforms or for so-called "flaky" tests to fail ("be red"). It is
Expand Down Expand Up @@ -553,16 +554,6 @@ After you push new changes to your branch, you need to get
approval for these new changes again, even if GitHub shows "Approved"
because the reviewers have hit the buttons before.

### CI testing

Every pull request needs to be tested
to make sure that it works on the platforms that Node.js
supports. This is done by running the code through the CI system.

Only a collaborator can start a CI run. Usually one of them will do it
for you as approvals for the pull request come in.
If not, you can ask a collaborator to start a CI run.

### Waiting until the pull request gets landed

A pull request needs to stay open for at least 48 hours from when it is
Expand Down Expand Up @@ -591,7 +582,7 @@ You can find the full list of supported subsystems in the
More than one subsystem may be valid for any particular issue or pull request.

[Building guide]: ../../BUILDING.md
[CI (Continuous Integration) test run]: #ci-testing
[CI (Continuous Integration) test run]: #continuous-integration-testing
[Code of Conduct]: https://github.com/nodejs/admin/blob/HEAD/CODE_OF_CONDUCT.md
[Onboarding guide]: ../../onboarding.md
[approved]: #getting-approvals-for-your-pull-request
Expand Down