Update docs about the pull-request-process (PowerShell#4710)

SteveL-MSFT · daxian-dbw · commit efa320cdf89b · 2017-09-04T10:29:20.000-07:00
Update pull-request-process to clarify role/responsibility of author and reviewers based on recent feedback from team and community. Also moved it to `.github/CONTRIBUTING.md` so that all related information can be found at one place.
diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
@@ -12,8 +12,8 @@ Please read the rest of this document to ensure a smooth contribution process.
 
 * Make sure you have a [GitHub account](https://github.com/signup/free).
 * Learning Git:
-  * GitHub Help: [Good Resources for Learning Git and GitHub][good-git-resources]
-  * [Git Basics](../docs/git/basics.md): install and getting started
+    * GitHub Help: [Good Resources for Learning Git and GitHub][good-git-resources]
+    * [Git Basics](../docs/git/basics.md): install and getting started
 * [GitHub Flow Guide](https://guides.github.com/introduction/flow/):
   step-by-step instructions of GitHub Flow
 
@@ -105,7 +105,7 @@ Additional references:
   Each commit should be a **single complete** change.
   This discipline is important when reviewing the changes as well as when using `git bisect` and `git revert`.
 
-#### Pull request submission
+#### Pull request - Submission
 
 **Always create a pull request to the `master` branch of this repository**.
 
@@ -136,8 +136,8 @@ Additional references:
   ```
 
 * Please use the present tense and imperative mood when describing your changes:
-  * Instead of "Adding support for Windows Server 2012 R2", write "Add support for Windows Server 2012 R2".
-  * Instead of "Fixed for server connection issue", write "Fix server connection issue".
+    * Instead of "Adding support for Windows Server 2012 R2", write "Add support for Windows Server 2012 R2".
+    * Instead of "Fixed for server connection issue", write "Fix server connection issue".
 
   This form is akin to giving commands to the code base
   and is recommended by the Git SCM developers.
@@ -167,50 +167,55 @@ Additional references:
   [run the spellchecker command line tool in interactive mode](#spellchecking-documentation)
   to add words to the `.spelling` file.
 
-#### Pull Request - Code Review
-
-* Roles and Responsibilities of a PR: Author, Reviewer, and Assignee
-  * Reviewer and Assignee are two separate roles of a PR.
-    * A Reviewer can be anyone who wants to contribute.
-      A Reviewer reviews the change of a PR,
-      leaves comments for the Author to address,
-      and approves the PR when the change looks good.
-    * An Assignee must be a [Maintainer](../docs/maintainers), who monitors the progress of the PR,
-      coordinates the review process, and merges the PR after it's been approved.
-      The Assignee may or may not be a Reviewer of the PR at the same time.
-  * An Author is encouraged to choose Reviewer(s) and an Assignee for the PR.
-    If no Assignee is chosen, one of the Maintainers shall be assigned to it.
-    If no Reviewer is chosen, the Assignee shall choose Reviewer(s) as appropriate.
-    * If an Author is a [PowerShell Team](https://github.com/orgs/PowerShell/people) member,
-      then the Author **is required** to choose Reviewer(s) and an Assignee for the PR.
-  * For a PR to be merged, it must be approved by at least one PowerShell Team member or Collaborator,
-    so additional Reviewer(s) may be added by the Assignee as appropriate.
-    The Assignee may also be re-assigned by Maintainers.
-* A Reviewer can postpone the code review if CI builds fail,
-  but also can start the code review early regardless of the CI builds.
-* The Author **is responsible** for driving the PR to the Approved state.
-  The Author addresses review comments, and pings Reviewer(s) to start the next iteration.
-  If the review is making no progress (or very slow),
-  the Author can always ask the Assignee to help coordinate the process and keep it moving.
-* Additional feedback is always welcome!
-  Even if you are not designated as a Reviewer,
-  feel free to review others' pull requests anyway.
-  Leave your comments even if everything looks good;
-  a simple "Looks good to me" or "LGTM" will suffice.
-  This way we know someone has already taken a look at it!
-* When updating your pull request, please **create new commits**
-  and **don't rewrite the commits history**. This way it's very easy for
-  the reviewers to see diff between iterations.
-  If you rewrite the history in the pull request, review could be much slower.
-  Once the review is done, you can rewrite the history to make it prettier,
-  if you like.
-  Otherwise it's likely would be squashed on merge to master.
-* Once the code review is done,
-  all merge conflicts are resolved,
-  and the CI system build status is passing,
-  the PR Assignee will merge your changes.
-* For more information on the PowerShell Maintainers' process,
-  see the [documentation](../docs/maintainers).
+#### Pull Request - Workflow
+
+1. The PR *author* creates a pull request from a fork.
+1. The *author* ensures that their pull request passes the [CI system][ci-system] build.
+   - If the build fails, a [Repository Maintainer][repository-maintainer] adds the `Review - waiting on author` label to the pull request.
+   The *author* can then continue to update the pull request until the build passes.
+1. If the *author* knows whom should participate in the review, they should add them otherwise they can add the recommended *reviewers*.
+1. Once the build passes, if there is not sufficient review, the *maintainer* adds the `Review - needed` label.
+1. An [Area Expert][area-expert] should also review the pull request.
+   - If the *author* does not meet the *reviewer*'s standards, the *reviewer* makes comments. A *maintainer* then removes the `Review - needed` label and adds
+   the `Review - waiting on author` label. The *author* must address the comments and repeat from step 2.
+   - If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.
+1. Once the code review is completed, a *maintainer* merges the pull request after one business day to allow for additional critical feedback.
+
+#### Pull Request - Roles and Responsibilities
+
+1. The PR *author* is responsible for moving the PR forward to get it Approved.
+   This includes addressing feedback within a timely period and indicating feedback has been addressed by adding a comment and mentioning the specific *reviewers*.
+   When updating your pull request, please **create new commits** and **don't rewrite the commits history**.
+   This way it's very easy for the reviewers to see diff between iterations.
+   If you rewrite the history in the pull request, review could be much slower.
+   The PR is likely to be squashed on merge to master by the *assignee*.
+1. *Reviewers* are anyone who wants to contribute.
+   They are responsible for ensuring the code: addresses the issue being fixed, does not create new issues (functional, performance, reliability, or security), and implements proper design.
+   *Reviewers* should use the `Review changes` drop down to indicate they are done with their review.
+   - `Request changes` if you believe the PR merge should be blocked if your feedback is not addressed,
+   - `Approve` if you believe your feedback has been addressed or the code is fine as-is, it is customary (although not required) to leave a simple "Looks good to me" (or "LGTM") as the comment for approval.
+   - `Comment` if you are making suggestions that the *author* does not have to accept.
+   Early in the review, it is acceptable to provide feedback on coding formatting based on the published [Coding Guidelines](../docs/dev-process/coding-guidelines.md), however,
+   after the PR has been approved, it is generally _not_ recommended to focus on formatting issues unless they go against the [Coding Guidelines](../docs/dev-process/coding-guidelines.md).
+   Non-critical late feedback (after PR has been approved) can be submitted as a new issue or new pull request from the *reviewer*.
+1. *Assignee* who are always *Maintainers* ensure that proper review has occurred and if they believe one approval is not sufficient, the *maintainer* is responsible to add more reviewers.
+   An *assignee* may also be a reviewer, but the roles are distinct.
+   Once the PR has been approved and the CI system is passing, the *assignee* will merge the PR after giving one business day for any critical feedback.
+   For more information on the PowerShell Maintainers' process, see the [documentation](../docs/maintainers).
+
+#### Pull Requests - Abandoned
+
+A pull request with the label `Review - waiting on author` for **more than two weeks** without a word from the author is considered abandoned.
+
+In these cases:
+
+1. *Assignee* will ping the author of PR to remind them of pending changes.
+   - If the *author* responds, it's no longer an abandoned; the pull request proceeds as normal.
+1. If the *author* does not respond **within a week**:
+   - If the *reviewer*'s comments are very minor, merge the change, fix the code immediately, and create a new PR with the fixes addressing the minor comments.
+   - If the changes required to merge the pull request are significant but needed, *assignee* creates a new branch with the changes and open an issue to merge the code into the dev branch.
+   Mention the original pull request ID in the description of the new issue and close the abandoned pull request.
+   - If the changes in an abandoned pull request are no longer needed (e.g. due to refactoring of the code base or a design change), *assignee* will simply close the pull request.
 
 ## Making Breaking Changes
 
@@ -235,7 +240,7 @@ we encourage contributors to follow these common engineering practices:
 
 * Format commit messages following these guidelines:
 
-```
+```text
 Summarize change in 50 characters or less
 
 Similar to email, this is the body of the commit message,
@@ -308,3 +313,6 @@ Once you sign a CLA, all your existing and future pull requests will be labeled
 [semantic linefeeds]: http://rhodesmill.org/brandon/2012/one-sentence-per-line/
 [PowerShell-Docs]: https://github.com/powershell/powershell-docs/
 [use-vscode-editor]: ../docs/learning-powershell/using-vscode.md#editing-with-visual-studio-code
+[repository-maintainer]: ../docs/community/governance.md#repository-maintainers
+[area-expert]: ../docs/community/governance.md#area-experts
+[ci-system]: ../docs/testing-guidelines/testing-guidelines.md#ci-system
diff --git a/docs/maintainers/README.md b/docs/maintainers/README.md
@@ -71,32 +71,7 @@ Please see [Issue Management][issue-management]
 
 ## Pull Request Workflow
 
-1. A contributor opens a pull request.
-1. The contributor ensures that their pull request passes the [CI system][ci-system] build.
-    - If the build fails, a maintainer adds the ```waiting for author``` label to the pull request.
-        The contributor can then continue to update the pull request until the build passes.
-1. Once the build passes, the maintainer either reviews the pull request immediately or adds the ```need review``` label.
-1. A maintainer or trusted contributor reviews the pull request code.
-    - If the contributor does not meet the reviewer's standards, the reviewer makes comments.
-        A maintainer then removes the ```need review``` label and adds the ```waiting for author``` label.
-    The contributor must address the comments and repeat from step 2.
-    - If the contributor meets the reviewer's standards, the reviewer comments that they are satisfied.
-        A maintainer then removes the ```need review``` label.
-1. Once the code review is completed, a maintainer merges the pull request.
-
-### Abandoned Pull Requests
-
-A pull request with the label ```waiting for the author``` for **more than two weeks** without a word from the author is considered abandoned.
-
-In these cases:
-
-1. Ping the author of PR to remind him of pending changes.
-    - If the contributor responds, it's no longer an abandoned pull request, proceed as normal.
-1. If the contributor does not respond **within a week**:
-    - Create a new branch with the changes and open an issue to merge the code into the dev branch.
-        Mention the original pull request ID in the description of the new issue and close the abandoned pull request.
-    - If the changes in an abandoned pull request are no longer needed (e.g. due to refactoring of
-        the code base or a design change), simply close the pull request.
+Please see [Contributing][CONTRIBUTING]
 
 ## Becoming a Repository Maintainer
 
diff --git a/docs/maintainers/pull-request-process.md b/docs/maintainers/pull-request-process.md
diff --git a/test/common/markdown/markdown.tests.ps1 b/test/common/markdown/markdown.tests.ps1
@@ -79,6 +79,7 @@ Describe 'Common Tests - Validate Markdown Files' -Tag 'CI' {
                 './demos/SSHRemoting/*.md'
                 './docker/*.md'
                 './tools/*.md'
+                './.github/CONTRIBUTING.md'
             )
             $filter = ($docsToTest -join ',')
             &"gulp" test-mdsyntax --silent `

Original file line number	Diff line number	Diff line change
`@@ -79,6 +79,7 @@ Describe 'Common Tests - Validate Markdown Files' -Tag 'CI' {`
`79`	`79`	`'./demos/SSHRemoting/*.md'`
`80`	`80`	`'./docker/*.md'`
`81`	`81`	`'./tools/*.md'`
	`82`	`+ './.github/CONTRIBUTING.md'`
`82`	`83`	`)`
`83`	`84`	`$filter = ($docsToTest -join ',')`
`84`	`85`	&"gulp" test-mdsyntax --silent `