Skip to content

Commit c1c6cce

Browse files
committed
merge pull request content into CONTRIBUTING.md
removed redundant info from maintainters README.md remove pull-request-process.md
1 parent 0cda0d9 commit c1c6cce

File tree

3 files changed

+55
-122
lines changed

3 files changed

+55
-122
lines changed

.github/CONTRIBUTING.md

Lines changed: 54 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ Additional references:
105105
Each commit should be a **single complete** change.
106106
This discipline is important when reviewing the changes as well as when using `git bisect` and `git revert`.
107107

108-
#### Pull request submission
108+
#### Pull request - Submission
109109

110110
**Always create a pull request to the `master` branch of this repository**.
111111

@@ -167,50 +167,56 @@ Additional references:
167167
[run the spellchecker command line tool in interactive mode](#spellchecking-documentation)
168168
to add words to the `.spelling` file.
169169

170-
#### Pull Request - Code Review
171-
172-
* Roles and Responsibilities of a PR: Author, Reviewer, and Assignee
173-
* Reviewer and Assignee are two separate roles of a PR.
174-
* A Reviewer can be anyone who wants to contribute.
175-
A Reviewer reviews the change of a PR,
176-
leaves comments for the Author to address,
177-
and approves the PR when the change looks good.
178-
* An Assignee must be a [Maintainer](../docs/maintainers), who monitors the progress of the PR,
179-
coordinates the review process, and merges the PR after it's been approved.
180-
The Assignee may or may not be a Reviewer of the PR at the same time.
181-
* An Author is encouraged to choose Reviewer(s) and an Assignee for the PR.
182-
If no Assignee is chosen, one of the Maintainers shall be assigned to it.
183-
If no Reviewer is chosen, the Assignee shall choose Reviewer(s) as appropriate.
184-
* If an Author is a [PowerShell Team](https://github.com/orgs/PowerShell/people) member,
185-
then the Author **is required** to choose Reviewer(s) and an Assignee for the PR.
186-
* For a PR to be merged, it must be approved by at least one PowerShell Team member or Collaborator,
187-
so additional Reviewer(s) may be added by the Assignee as appropriate.
188-
The Assignee may also be re-assigned by Maintainers.
189-
* A Reviewer can postpone the code review if CI builds fail,
190-
but also can start the code review early regardless of the CI builds.
191-
* The Author **is responsible** for driving the PR to the Approved state.
192-
The Author addresses review comments, and pings Reviewer(s) to start the next iteration.
193-
If the review is making no progress (or very slow),
194-
the Author can always ask the Assignee to help coordinate the process and keep it moving.
195-
* Additional feedback is always welcome!
196-
Even if you are not designated as a Reviewer,
197-
feel free to review others' pull requests anyway.
198-
Leave your comments even if everything looks good;
199-
a simple "Looks good to me" or "LGTM" will suffice.
200-
This way we know someone has already taken a look at it!
201-
* When updating your pull request, please **create new commits**
202-
and **don't rewrite the commits history**. This way it's very easy for
203-
the reviewers to see diff between iterations.
204-
If you rewrite the history in the pull request, review could be much slower.
205-
Once the review is done, you can rewrite the history to make it prettier,
206-
if you like.
207-
Otherwise it's likely would be squashed on merge to master.
208-
* Once the code review is done,
209-
all merge conflicts are resolved,
210-
and the CI system build status is passing,
211-
the PR Assignee will merge your changes.
212-
* For more information on the PowerShell Maintainers' process,
213-
see the [documentation](../docs/maintainers).
170+
#### Pull Request - Workflow
171+
172+
1. The PR *author* creates a pull request from a fork.
173+
1. The *author* ensures that their pull request passes the [CI system][ci-system] build.
174+
- If the build fails, a [Repository Maintainer][repository-maintainer] adds the `Review - waiting on author` label to the pull request.
175+
The *author* can then continue to update the pull request until the build passes.
176+
1. If the *author* knows whom should participate in the review, they should add them otherwise they can add the recommended *reviewers*.
177+
1. Once the build passes, if there is not sufficient review, the *maintainer* adds the `Review - needed` label.
178+
1. An [Area Expert][area-expert] should also review the pull request.
179+
- If the *author* does not meet the *reviewer*'s standards, the *reviewer* makes comments. A *maintainer* then removes the `Review - needed` label and adds
180+
the `Review - waiting on author` label. The *author* must address the comments and repeat from step 2.
181+
- If the *author* meets the *reviewer*'s standards, the *reviewer* approves the PR. A maintainer then removes the `need review` label.
182+
1. Once the code review is completed, a *maintainer* merges the pull request after one business day to allow for additional critical feedback.
183+
184+
#### Pull Request - Roles and Responsibilities
185+
186+
1. The PR *author* is responsible for moving the PR forward to get it Approved.
187+
This includes addressing feedback within a timely period and indicating feedback has been addressed by adding a comment and mentioning the specific *reviewers*.
188+
When updating your pull request, please **create new commits** and **don't rewrite the commits history**.
189+
This way it's very easy for the reviewers to see diff between iterations.
190+
If you rewrite the history in the pull request, review could be much slower.
191+
Once the review is done, you can rewrite the history to make it prettier, if you like.
192+
Otherwise it's likely would be squashed on merge to master by the *assignee*.
193+
1. *Reviewers* are any anyone who wants to contribute.
194+
They are responsible for ensuring the code: addresses the issue being fixed, does not create new issues (functional, performance, or reliability), and implements proper design.
195+
*Reviewers* should use the `Review changes` drop down to indicate they are done with their review.
196+
- `Request changes` if you believe the PR merge should be blocked if your feedback is not addressed,
197+
- `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.
198+
- `Comment` if you are making suggestions that the *author* does not have to accept.
199+
Early in the review, it is acceptable to provide feedback on coding style based on the published [Coding Guidelines](../docs/dev-process/coding-guidelines.md), however, after
200+
the *author* has already pushed commits to address feedback, it is generally _not_ acceptable to focus on style issues as it disrupts the PR process.
201+
Non-critical late feedback (after PR has been approved) can be submitted as a new issue or new pull request from the *reviewer*.
202+
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.
203+
An *assignee* may also be a reviewer, but the roles are distinct.
204+
Once the PR has been approved and the CI system is passing, the *assignee* will merge the PR.
205+
For more information on the PowerShell Maintainers' process, see the [documentation](../docs/maintainers).
206+
207+
#### Pull Requests - Abandoned
208+
209+
A pull request with the label `Review - waiting on author` for **more than two weeks** without a word from the author is considered abandoned.
210+
211+
In these cases:
212+
213+
1. *Assignee* will ping the author of PR to remind them of pending changes.
214+
- If the *author* responds, it's no longer an abandoned; the pull request proceeds as normal.
215+
1. If the *author* does not respond **within a week**:
216+
- 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.
217+
- 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.
218+
Mention the original pull request ID in the description of the new issue and close the abandoned pull request.
219+
- 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.
214220

215221
## Making Breaking Changes
216222

@@ -308,3 +314,6 @@ Once you sign a CLA, all your existing and future pull requests will be labeled
308314
[semantic linefeeds]: http://rhodesmill.org/brandon/2012/one-sentence-per-line/
309315
[PowerShell-Docs]: https://github.com/powershell/powershell-docs/
310316
[use-vscode-editor]: ../docs/learning-powershell/using-vscode.md#editing-with-visual-studio-code
317+
[repository-maintainer]: ../docs/community/governance.md#repository-maintainers
318+
[area-expert]: ../docs/community/governance.md#area-experts
319+
[ci-system]: ../docs/testing-guidelines/testing-guidelines.md#ci-system

docs/dev-process/pull-request-process.md

Lines changed: 0 additions & 51 deletions
This file was deleted.

docs/maintainers/README.md

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -71,32 +71,7 @@ Please see [Issue Management][issue-management]
7171

7272
## Pull Request Workflow
7373

74-
1. A contributor opens a pull request.
75-
1. The contributor ensures that their pull request passes the [CI system][ci-system] build.
76-
- If the build fails, a maintainer adds the ```waiting for author``` label to the pull request.
77-
The contributor can then continue to update the pull request until the build passes.
78-
1. Once the build passes, the maintainer either reviews the pull request immediately or adds the ```need review``` label.
79-
1. A maintainer or trusted contributor reviews the pull request code.
80-
- If the contributor does not meet the reviewer's standards, the reviewer makes comments.
81-
A maintainer then removes the ```need review``` label and adds the ```waiting for author``` label.
82-
The contributor must address the comments and repeat from step 2.
83-
- If the contributor meets the reviewer's standards, the reviewer comments that they are satisfied.
84-
A maintainer then removes the ```need review``` label.
85-
1. Once the code review is completed, a maintainer merges the pull request.
86-
87-
### Abandoned Pull Requests
88-
89-
A pull request with the label ```waiting for the author``` for **more than two weeks** without a word from the author is considered abandoned.
90-
91-
In these cases:
92-
93-
1. Ping the author of PR to remind him of pending changes.
94-
- If the contributor responds, it's no longer an abandoned pull request, proceed as normal.
95-
1. If the contributor does not respond **within a week**:
96-
- Create a new branch with the changes and open an issue to merge the code into the dev branch.
97-
Mention the original pull request ID in the description of the new issue and close the abandoned pull request.
98-
- If the changes in an abandoned pull request are no longer needed (e.g. due to refactoring of
99-
the code base or a design change), simply close the pull request.
74+
Please see [Contributing][CONTRIBUTING]
10075

10176
## Becoming a Repository Maintainer
10277

0 commit comments

Comments
 (0)