doc: onboarding.md landing prs multiple commits #9635

BethGriggs · 2016-11-16T10:21:49Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

onboarding.md and COLLABORATOR_GUIDE.md cover much of the same ground, this PR aims to remove some of the duplicated content to avoid ambiguity.

gibfahn · 2016-11-16T11:03:54Z

cc/ @sam-github

sam-github · 2016-11-16T18:00:39Z

doc/onboarding.md

@@ -203,7 +203,7 @@ Landing a PR
      * Full URL of material that might provide additional useful information or context to someone trying to understand the change set or the thinking behind it.
 * Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. Only do this when landing your own contributions.
 * `git push upstream master`
-    * Close the pull request with a "Landed in `<commit hash>`" comment.
+    * Close the pull request with a "Landed in `<commit hash>`" comment. If Github automatically merged your pull request then you should still add the "Landed in <commit hash>..<commit hash>" comment if you added multiple commits.


Landed in is not required according to https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#landing-pull-requests.

These two documents should agree. Either by making them say the same thing, or by making onboarding be a reference to the collaborator guide.

Its not clear what "automatically merged" means. github never automatically merges anything, unless you mean when the green button is pressed by a human. I think you mean "when github detects the branch was merged"? In which case, you probably have to discuss the subtleties of how it does that. And maybe mention that a Landed in is never the wrong thing?

cc: @Fishrock123

@sam-github As in: if you push a commit to master that has the same hash as the HEAD of a PR branch, then Github will automatically mark the PR as merged. I guess the wording should be more exact.

ok, so subtle is in the eye of the beholder when it comes to git :-), but I don't think "github automatically merged your commit" is the same as "the branches commits were fast-forwarded onto master so the master contains the exact SHA commit IDs as the PR branch did". Anyhow, point is, if there is a documented exception, the docs should go a bit more into it, perhaps recommending force pushing your own branches since you can, but pointing out that's not usually possible with branches you don't own.

I'll look at rewriting this to make it clearer, perhaps to make more of a distinction between COLLABORATOR_GUIDE and onboarding.md (#9555 (comment)).

sam-github · 2016-11-18T18:03:33Z

COLLABORATOR_GUIDE.md

+
+  * Easiest to use `git log`, then do a search.
+  * In vim: `/Name` + `enter` (+ `n` as much as you need to)
+  * Only include collaborators who have commented `LGTM`.


... " or approved the request using the github review mechanism."

I guess de facto it's allowed, but I'm not sure discussion on using it ever reached any kind of consensus (ref: #8554).

sam-github · 2016-11-18T18:08:57Z

COLLABORATOR_GUIDE.md

@@ -230,6 +247,11 @@ Time to push it:
 ```text
 $ git push origin master
 ```
+* Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. Only do this when landing your own contributions.


"When the pull request is closed" ---> "When the amended commit is fast-forward merged onto master, github will automatically close the PR, and mark it with the purple merge status"....

You text made it sound like if you close a PR using the "close pull request" button that it would show as merged, but you mean "closing a PR in the way node does it"

sam-github · 2016-11-18T18:14:35Z

COLLABORATOR_GUIDE.md

 Always modify the original commit message to include additional meta
 information regarding the change process:

 - A `Reviewed-By: Name <email>` line for yourself and any
  other Collaborators who have reviewed the change.
  - Useful for @mentions / contact list if something goes wrong in the PR.
  - Protects against the assumption that GitHub will be around forever.
+
+  * Easiest to use `git log`, then do a search.
+  * In vim: `/Name` + `enter` (+ `n` as much as you need to)


I think you mean the "default git pager, less" here, not vim? Usually git log, when output is longer than the terminal uses the git pager, which defaults to less, and less (as well as the old pager, more, vim, and lots of tools) uses / for searching.

This was copied from https://github.com/nodejs/node/blob/master/doc/onboarding.md#landing-prs-details.

I agree it should be less.

targos · 2016-11-19T09:40:08Z

COLLABORATOR_GUIDE.md

 Always modify the original commit message to include additional meta
 information regarding the change process:

 - A `Reviewed-By: Name <email>` line for yourself and any
  other Collaborators who have reviewed the change.
  - Useful for @mentions / contact list if something goes wrong in the PR.
  - Protects against the assumption that GitHub will be around forever.
+
+  * Easiest to use `git log`, then do a search.


https://github.com/evanlucas/node-review can help a lot to do this list

I agree that we should officially document this

lpinca · 2016-11-19T20:34:05Z

COLLABORATOR_GUIDE.md

@@ -230,6 +247,11 @@ Time to push it:
 ```text
 $ git push origin master
 ```
+* Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. Only do this when landing your own contributions.
+
+* Close the pull request with a "Landed in `<commit hash>`" comment. If Github automatically merged your pull request then you should still add the "Landed in <commit hash>..<commit hash>" comment if you added multiple commits.


Nit: Github -> GitHub

gibfahn · 2016-11-22T13:43:45Z

doc/onboarding.md

    * For raw commit message: `git log 7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 -1`
  * Collaborators are in alphabetical order by GitHub username.
  * Label your pull request with the `doc` subsystem label.
  * Run CI on your PR.
  * After a `LGTM` or two, land the PR.
    * Be sure to add the `PR-URL: <full-pr-url>` and appropriate `Reviewed-By:` metadata!

-
-## final notes
+## Final notes

  * don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!)
  * very few (no?) mistakes are unrecoverable
  * the existing node committers trust you and are grateful for your help!


s/committers/Collaborators

gibfahn · 2016-11-22T13:49:33Z

COLLABORATOR_GUIDE.md

+- A `Reviewed-By: Name <email>` line for yourself and any
+  other Collaborators who have reviewed the change.
+  - Useful for @mentions / contact list if something goes wrong in the PR.
+  - Protects against the assumption that GitHub will be around forever.

 Review the commit message to ensure that it adheres to the guidelines
 outlined in the [contributing](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit) guide.


Probably worth mentioning https://github.com/evanlucas/core-validate-commit and https://github.com/evanlucas/node-review here.

gibfahn · 2016-11-22T13:53:35Z

COLLABORATOR_GUIDE.md

+
+* `./configure && make -j8 test` (`-j8` builds node in parallel with 8
+  threads. Adjust to the number of cores (or processor-level threads)
+  your processor has (or slightly more) for best results.)


I think this is an excessive number of brackets.

(-j8 builds node in parallel with 8 threads. Adjust to the number of processor cores/threads your machine has for best results.)

COLLABORATOR_GUIDE.md and onboarding.md cover some of the same information. The aim of this commit is to remove duplicated information.

BethGriggs · 2016-12-05T10:45:05Z

Updated, PTAL. I have not included the changes to document evanlucas/node-review as I feel this should be part of a separate PR.

sam-github · 2016-12-06T17:43:46Z

Landed in 8951d3e

COLLABORATOR_GUIDE.md and onboarding.md cover some of the same information. The aim of this commit is to remove duplicated information. PR-URL: #9635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

COLLABORATOR_GUIDE.md and onboarding.md cover some of the same information. The aim of this commit is to remove duplicated information. PR-URL: nodejs#9635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

COLLABORATOR_GUIDE.md and onboarding.md cover some of the same information. The aim of this commit is to remove duplicated information. PR-URL: #9635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 16, 2016

sam-github suggested changes Nov 16, 2016

View reviewed changes

gibfahn added the wip Issues and PRs that are still a work in progress. label Nov 17, 2016

BethGriggs force-pushed the pr-onboarding-multi-commits branch from eee8811 to 346f675 Compare November 17, 2016 16:37

jasnell approved these changes Nov 18, 2016

View reviewed changes

sam-github reviewed Nov 18, 2016

View reviewed changes

targos reviewed Nov 19, 2016

View reviewed changes

lpinca reviewed Nov 19, 2016

View reviewed changes

BethGriggs force-pushed the pr-onboarding-multi-commits branch 13 times, most recently from e701f36 to c1cbe63 Compare November 22, 2016 13:41

gibfahn reviewed Nov 22, 2016

View reviewed changes

doc: remove repeated info onboarding.md

1845c1d

COLLABORATOR_GUIDE.md and onboarding.md cover some of the same information. The aim of this commit is to remove duplicated information.

BethGriggs force-pushed the pr-onboarding-multi-commits branch from c1cbe63 to 1845c1d Compare December 5, 2016 10:38

jasnell approved these changes Dec 5, 2016

View reviewed changes

sam-github approved these changes Dec 6, 2016

View reviewed changes

sam-github closed this Dec 6, 2016

MylesBorins added land-on-v6.x labels Dec 20, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

BethGriggs deleted the pr-onboarding-multi-commits branch February 21, 2018 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: onboarding.md landing prs multiple commits #9635

doc: onboarding.md landing prs multiple commits #9635

BethGriggs commented Nov 16, 2016 •

edited

Loading

gibfahn commented Nov 16, 2016

sam-github Nov 16, 2016

sam-github Nov 16, 2016

gibfahn Nov 16, 2016 •

edited

Loading

sam-github Nov 16, 2016

BethGriggs Nov 17, 2016

sam-github Nov 18, 2016

gibfahn Nov 19, 2016

sam-github Nov 18, 2016

sam-github Nov 18, 2016

sam-github Nov 18, 2016

gibfahn Nov 19, 2016

targos Nov 19, 2016

gibfahn Nov 19, 2016

lpinca Nov 19, 2016

gibfahn Nov 22, 2016

gibfahn Nov 22, 2016

gibfahn Nov 22, 2016

BethGriggs commented Dec 5, 2016

sam-github commented Dec 6, 2016

doc: onboarding.md landing prs multiple commits #9635

doc: onboarding.md landing prs multiple commits #9635

Conversation

BethGriggs commented Nov 16, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

gibfahn commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BethGriggs commented Dec 5, 2016

sam-github commented Dec 6, 2016

BethGriggs commented Nov 16, 2016 •

edited

Loading

gibfahn Nov 16, 2016 •

edited

Loading