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: clarify the review and landing process #10202

Closed
wants to merge 10 commits into from
Closed

doc: clarify the review and landing process #10202

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cc22d35
doc: clarify the review and landing process
joyeecheung Dec 9, 2016
e8153b7
doc: clarify the landing process
joyeecheung Dec 11, 2016
0bb76ba
doc: explain the waiting time of semver-major PRs
joyeecheung Dec 13, 2016
32ab27a
doc: put the notes and details in the back
joyeecheung Dec 14, 2016
9bda4a4
doc: improve the clarification
joyeecheung Dec 15, 2016
dbd83ef
doc: more fixes about the process
joyeecheung Dec 15, 2016
96e0189
doc: amends the contributing guide
joyeecheung Dec 16, 2016
246f52a
doc: testing docs, PR history preservation
joyeecheung Dec 16, 2016
e9c53f3
doc: fix typo
joyeecheung Dec 20, 2016
41e4e93
doc: trim trailling spaces
joyeecheung Dec 22, 2016
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
85 changes: 77 additions & 8 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,18 +243,87 @@ If in doubt, you can always ask for guidance in the Pull Request or on
[IRC in the #node-dev channel](https://webchat.freenode.net?channels=node-dev&uio=d4).

Feel free to post a comment in the Pull Request to ping reviewers if you are
awaiting an answer on something.
awaiting an answer on something. If you encounter words or acronyms that
seem unfamiliar, check out this
[glossary](https://sites.google.com/a/chromium.org/dev/glossary).
Copy link
Member

Choose a reason for hiding this comment

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

Oh… thank you! I didn’t know this existed.


Note that multiple commits often get squashed when they are landed (see the
notes about [commit squashing](#commit-squashing)).

### Step 8: Landing

Once your Pull Request has been reviewed and approved by at least one Node.js
Collaborators (often by saying LGTM, or Looks Good To Me), and as long as
there is consensus (no objections from a Collaborator), a
Collaborator can merge the Pull Request . GitHub often shows the Pull Request as
`Closed` at this point, but don't worry. If you look at the branch you raised
your Pull Request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!
In order to get landed, a Pull Request needs to be reviewed and
[approved](#getting-approvals-for-your-pull-request) by
at least one Node.js Collaborator. If the Pull Request touches
more than the documentation, then it also need to pass a
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern with this is the addon tests are actually generated by the documentation. I do recall a time where our test suite was broken due to addon docs being changed and merged without the CI being run. Maybe worth mentioning?

Copy link
Member

@gibfahn gibfahn Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

TBH I feel that it's worth running the CI even on doc changes, otherwise there's no record that the linter was run. I know that collaborators are supposed to run make lint as part of the landing process, but that inevitably doesn't always happen, and it has caused issues in the past.

Copy link
Member Author

@joyeecheung joyeecheung Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I didn't know about this! Thanks for the explanation, maybe I should also remind new contributors about this in the Step 5: Test section? So is it like:

  • If you touch the code snippets inside addons.md, you need to run make -j8 test
  • If you touch the code snippets inside any documentation, you need to run make lint

Is there anything else that can be added to this list?

Copy link
Member

Choose a reason for hiding this comment

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

make -j4 test does run the addons tests, and we currently tell everyone to run that on their PR. I'd prefer just leaving it like that, we don't want to be telling people that they don't need to run the full test suite (they'll work that out for themselves).

Copy link
Member Author

@joyeecheung joyeecheung Dec 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So the instructions here should be something like "as long as you touch the code snippets inside the documentation, you need to run make test"? Or just leave out the documentation part and state that every PR, including doc PR, needs to pass tests?

Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung So currently in the guide it says that:

Make sure the linter is happy and that all tests pass. Please, do not submit patches that fail either check.

I think that this sentence is sufficient, it says that you should always make sure the tests and linter pass.

Copy link
Member Author

@joyeecheung joyeecheung Dec 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So for this part that we are commenting on, I will just change this back to "..and pass a CI test run". Maybe I can add some notes at the end to remind people of the needs of running tests for changes to doc? Just feeling odd to let this practice go undocumented.

Copy link
Member

Choose a reason for hiding this comment

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

Well, what is currently documented is that you should always run make -j4 test no matter what, which is strictly correct. What isn't documented is that in practice you can sometimes get away with just running the linter (or the specific test you changed). That's not something we really want to encourage, especially as it's a tricky issue (some doc changes do need make test, some test changes do need make test). Documenting something is effectively legitimising it, which I (personally) don't think we want to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see your point, but what I meant was to stress that doc PR needs to pass the tests too (and possibly explain why , or not if that expose too much details). I didn't mean to remind people that there is a way to skip the tests. Because I think the previous section is not very clear about doc PR needs tests too. After all it could be unexpected to new comers that you need to set up the whole compiler tool chain to get some markdown changes pass the review. But they do need to, so a heads up could be nice.

That said, since in practice this could be flexible, I think I can leave this as-is. Thank you for the clarification!

[CI (Continuous Integration) test run](#ci-testing). After that,
as long as there are no objections from a Collaborator, the
Pull Request can be merged. If you find your Pull Request waiting
longer than you expect, see the
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).

When a collaborator lands your Pull Request, they will post
a comment to the Pull Request page mentioning the commit(s) it
landed as. GitHub often shows the Pull Request as `Closed` at this
point, but don't worry. If you look at the branch you raised your
Pull Request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!

## Additional Notes

### Commit Squashing

When the commits in your Pull Request get landed, they will be squashed
into one commit per logical change, with metadata added to the commit
message (including links to the Pull Request, links to relevant issues,
and the names of the reviewers). As long as you don't delete
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth linking to the COLLABORATOR_GUIDE.md somewhere in this document, so people can see what Collaborators actually do. Maybe link it here?

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 think it can be placed at the end of the "Additional Notes"?

your fork branch, the commit history of your Pull Request will stay
intact on the Pull Request page.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see the use case for keeping the commit history of the Pull Request. Obviously the comment history will be left for posterity, but a lot of people delete their PR branch as soon as it has been merged in.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a reminder telling people if they delete their fork branch, the commit history will be lost, should other people want to see it later. Sometimes the commit history might be useful if the PR is big.

Copy link
Member

Choose a reason for hiding this comment

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

I think github retains the commits for PRs even if the branch or the entire fork it came from is deleted? (Feel free to ignore me if you’ve already checked this.)

Copy link
Member Author

@joyeecheung joyeecheung Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

(I remember I've replied here before, was I typing in my dreams? Anyway I will just reply again)

This one is suggested in #10202 (comment) . I tried to delete and prune one of my branches in my remote repo and the remote commits still stick around, so they still show up in the PR. I don't know how this works exactly but I've been seeing some merged PRs without commit history, like this. Maybe @gibfahn knows how this works?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how this works exactly but I've been seeing some merged PRs without commit history, like this.

I think that is because in that specific example Eugene force-pushed the same commit to master and then to the PR, to get the purple “Merged” status?

Copy link
Member Author

Choose a reason for hiding this comment

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

:O Oh wow, I think people who know how this works this don't need the reminder here. Maybe I should change this back to

The commit history of your Pull Request, however, will stay intact on the Pull Request page.

Or is this reminder just unnecessary?


For the size of "one logical change",
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
can be a good example. It touches the implementation, the documentation,
and the tests, but is still one logical change.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a good place to mention that the tests should pass after each individual commit that lands on master?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I'll mention that here.


### Getting Approvals for Your Pull Request

A Pull Request is approved either by saying LGTM, which stands for
"Looks Good To Me", or by using GitHub's Approve button.
GitHub's Pull Request review feature can be used during the process.
For more information, check out
[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g)
or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/).

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

If your Pull Request contains executable code, it needs to be tested
to make sure the code works on the platforms that Node.js
supports. This is done by running the code through the CI system.

Only a Collaborator can request 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 request a CI run.

### Waiting Until the Pull Request Gets Landed

After the Pull Request gets approved and passes the CI,
it still needs to wait for at least another 48 hours (72
hours on a weekend) so that everyone has a chance to
weigh in. If the changes are trivial, collaborators may
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is is quite right. It has to wait 48 to 72 hours from when it is submitted not from when it passed the CI or is approved. I think if the PR has been around for that long and there are only minor changes addressing comments/requested changes then it can land. Specifically for the CI run its better to land soon after the CI run so that there is less change that interaction with some other change can cause a failure when it lands.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I will change that to "A Pull Request needs to stay open for....from when it is submitted, even after it gets approved and passes the CI".

decide it doesn't need to wait. A Pull Request may well take
longer to be merged in.
All these precautions are important because Node.js is widely used,
so don't be discouraged!

### Check Out the Collaborator's Guide

If you want to know more about the code review and the landing process,
you can take a look at the
[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md).

<a id="developers-certificate-of-origin"></a>
## Developer's Certificate of Origin 1.1
Expand Down