Define roles and their responsibilities in a PR #3168

daxian-dbw · 2017-02-17T21:24:36Z

Reviewer and Assignee of a PR should be separate roles.

Reviewers do code review, and eventually approve the PR
Assignee is a maintainer, who monitors the progress of the PR, coordinates the review process, and merges PR after it's approved.
When submitting a new PR, the author is encouraged to choose Reviewer(s) and Assignee for the PR. If either Reviewer or Assignee is left blank, maintainers will choose them for the PR.
However, for PowerShell Team members, You are required to choose them yourself for the PR.
Note that, even if you are not designated as a Reviewer for a PR, you can still feel free to jump in and review the change.

The PR author is responsible to drive the PR to the Approved state. The author should ping the reviewer(s) when they don't respond timely enough (or event talk to them if you can). If the review process is making no progress (or too slow), the author can always contact the Assignee to help coordinate the process, for example, if the reviewer(s) don't respond back for too long, the Assignee can review the existing comments and decide if the PR is in good state or a new reviewer should be pulled in.

A template will be added to pull request, to remind the author to choose Reviewer(s) and Assignee for the PR.

TravisEz13

I found no real conceptual issues. Just many grammar issues. Not sure I commented on all of them...

TravisEz13 · 2017-02-17T21:34:34Z

.github/CONTRIBUTING.md

+* Author **is responsible** to drive the PR to the Approved state.
+  After a successful test pass, Reviewer(s) can start a code review,
  commenting on any changes that might need to be made.
+  Author addresses the comments, and ping Reviewer(s) to start the nexe iteration.


spelling nexe should the next

TravisEz13 · 2017-02-17T21:36:42Z

.github/CONTRIBUTING.md


-* After a successful test pass,
-  the area maintainers will do a code review,
+* Author, Reviewer and Assignee of a PR


Author, Reviewer, and Assignee of a PR

TravisEz13 · 2017-02-17T21:37:10Z

.github/CONTRIBUTING.md

+* Author, Reviewer and Assignee of a PR
+  * Reviewer and Assignee are two separate roles of a PR.
+    * A Reviewer can be anyone from the collaborators.
+      Reviewer reviews the change of a PR,


unneeded comma

TravisEz13 · 2017-02-17T21:38:00Z

.github/CONTRIBUTING.md

+      Reviewer reviews the change of a PR,
+      leaves comments for Author to address,
+      and approve the PR when the change looks good.
+    * An Assignee must be a maintainer, who monitors progress of the PR,


missing article, should be the progress

TravisEz13 · 2017-02-17T21:39:05Z

.github/CONTRIBUTING.md

+      and approve the PR when the change looks good.
+    * An Assignee must be a maintainer, who monitors progress of the PR,
+      coordinates the review process, and merges the PR after it's approved.
+      Assignee may or may not be a Reviewer of the PR at the same time.


missing article, should be either The assignee or An assignee

TravisEz13 · 2017-02-17T21:39:51Z

.github/CONTRIBUTING.md

+  * Assignee may be re-assigned by maintainers,
+    and more Reviewer(s) may be added by Assignee as appropriate.
+
+* Author **is responsible** to drive the PR to the Approved state.


missing article on Author

to drive should be for driving

TravisEz13 · 2017-02-17T21:41:21Z

.github/CONTRIBUTING.md

+      Assignee may or may not be a Reviewer of the PR at the same time.
+  * Author is encouraged to choose Reviewer(s) and Assignee for the PR.
+    If no Assignee is chosen, one of maintainers will be assigned to it.
+    If no Reviewer is chosen, Assignee will choose Reviewer(s) as appropriate.


will be assigned to shall be assigned

will choose to shall choose

daxian-dbw · 2017-02-17T22:22:36Z

@TravisEz13 thanks for catching them. I think I fixed them all this time, can you take a look again?

TravisEz13 · 2017-02-17T23:48:31Z

.github/CONTRIBUTING.md

+* Author, Reviewer, and Assignee of a PR
+  * Reviewer and Assignee are two separate roles of a PR.
+    * A Reviewer can be anyone from the collaborators.
+      A Reviewer reviews the change of a PR,


comma at end of line 175 is not needed

If I remove that comma, that line will change from

reviews the change of a PR, leaves comments

to

reviews the change of a PR leaves comments

That doesn't seem right, does it?

Oh didn't catch that you were missing an And.

discussed offline and it looks good .

TravisEz13 · 2017-02-17T23:55:34Z

.github/PULL_REQUEST_TEMPLATE.md

+<!--
+
+If you are a [PowerShell Team](https://github.com/orgs/PowerShell/people) member, please make sure you choose the Reviewer(s) and Assignee for your PR.
+If you are not, feel free to choose them if you are familiar with the team. Or leave them blank and let maintainers choose them for you.


If you are not a team member

Replace Or leave with Alternatively,

After Or Replace the first Them with the fields.

Much better. Will fix it.

mirichmo · 2017-02-18T01:52:43Z

.github/CONTRIBUTING.md


-* After a successful test pass,
-  the area maintainers will do a code review,
+* Author, Reviewer, and Assignee of a PR


Can you make this heading a little more descriptive? Something like, "Roles of a PR: Author, Reviewer, and Assignee"

That is better.

mirichmo · 2017-02-18T01:57:19Z

.github/CONTRIBUTING.md

-  the area maintainers will do a code review,
+* Author, Reviewer, and Assignee of a PR
+  * Reviewer and Assignee are two separate roles of a PR.
+    * A Reviewer can be anyone from the collaborators.


This is a little misleading to me. "collaborators" are external people who have special status with the project (Charles, Ilya, Jeff, etc.). Do you mean that anyone can be a reviewer?

On a related note, should we specify that at least one PowerShell team member or a Collaborator must sign off for a review to be Approved?

Contributor is the more generic term

"collaborators" is the wrong word to use here.
When clicking the "Reviewer" field, is that list of people in fact (team member + collaborator)?
I think the author has the freedom to choose anyone from that list. Assignee needs to decide if the specified reviewer is sufficient. If not, the assignee can add more reviewers.

at least one PowerShell team member or a Collaborator must sign off for a review to be Approved

This information is added. Please take another look.

daxian-dbw · 2017-02-20T22:15:16Z

@mirichmo @TravisEz13 the conversation about "collaborators" got hidden by Github, but that comment still stands.

vors

Tentatively approved

vors · 2017-02-20T22:26:56Z

.github/CONTRIBUTING.md

+  * The Assignee may be re-assigned by maintainers,
+    and more Reviewer(s) may be added by Assignee as appropriate.
+
+* An Author **is responsible** for driving the PR to the Approved state.


vors · 2017-02-20T22:28:49Z

.github/CONTRIBUTING.md

+    and more Reviewer(s) may be added by Assignee as appropriate.
+
+* An Author **is responsible** for driving the PR to the Approved state.
+  After a successful test pass, Reviewer(s) can start a code review,


Reviewer can choose to start review before the successful test pass. I think we should re-phrase it to emphasize that reviwer can postpone reviewer, if tests are failing, but can start earlier.

Especially with our flaky tests.

Good point. Fixed.

vors · 2017-02-20T22:29:55Z

.github/CONTRIBUTING.md

+  The Author addresses the 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.



Should we mention labels that indicates who is currently responsible for the next step?

I think labels are more useful to maintainers and less useful to reviewers because a reviewer probably will not look at the PR list very often. So I think it's better that the author pings the reviewers when she/he is ready to start the next review iteration.

andyleejordan

I approve the content of these changes, but agree with Sergei's comments. This is excellent clarification I think.

SteveL-MSFT · 2017-02-21T16:58:20Z

.github/CONTRIBUTING.md


-* After a successful test pass,
-  the area maintainers will do a code review,
+* Roles of a PR: Author, Reviewer, and Assignee


Title should be:

Roles and Responsibilities of a PR: Author, Reviewer, and Assignee

SteveL-MSFT · 2017-02-21T16:59:22Z

.github/CONTRIBUTING.md

-  the area maintainers will do a code review,
+* Roles of a PR: Author, Reviewer, and Assignee
+  * Reviewer and Assignee are two separate roles of a PR.
+    * A Reviewer can be anyone from the collaborators.


A Reviewer can be anyone who wants to contribute

Probably also mention that PRs with Needs Review label needs a Reviewer

Changed to A Reviewer can be anyone who wants to contribute.

Probably also mention that PRs with Needs Review label needs a Reviewer

Do you mean the Review - Needed label? I'm afraid this label doesn't mean a reviewer needs to be assigned to a PR. I explicitly requested reviewers in #3170, but the Review - Needed label still got added to that PR.

Yes, I meant Review - Needed. I was told (forgot by whom) that was what this label meant. If not, we should document what exactly this label means (the current docs just has a link another doc without really defining this label)

SteveL-MSFT · 2017-02-21T17:00:12Z

.github/CONTRIBUTING.md

+      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, who monitors the progress of the PR,


Maintainer should be caps, add link to https://github.com/PowerShell/PowerShell/tree/master/docs/maintainers

SteveL-MSFT · 2017-02-21T17:00:39Z

.github/CONTRIBUTING.md

+      leaves comments for the Author to address,
+      and approves the PR when the change looks good.
+    * An Assignee must be a maintainer, who monitors the progress of the PR,
+      coordinates the review process, and merges the PR after it's approved.


"after it's been approved"

daxian-dbw · 2017-02-21T18:28:42Z

Hi Reviewers, I think I have addressed all the comments, can you please take another look?

joeyaiello

Not a huge deal if these tweaks get made before merge, but if you have the chance, it'd be awesome to fix them.

Overall, this looks awesome and provides some much needed clarity with the process.

joeyaiello · 2017-02-21T23:42:37Z

.github/PULL_REQUEST_TEMPLATE.md

+<!--
+
+If you are a [PowerShell Team](https://github.com/orgs/PowerShell/people) member, please make sure you choose the Reviewer(s) and Assignee for your PR.
+If you are not, feel free to choose them if you are familiar with the team. Alternatively, leave the fields blank and let maintainers choose them for you.


I would switch this sentence around as the "familiar" path is less likely. So something like:

If you are not part of the PowerShell Team, you can leave the fields blank and the Maintainers will choose them for you. If you are familiar with the team, feel free to add some Reviewers yourself.

joeyaiello · 2017-02-21T23:44:06Z

.github/CONTRIBUTING.md

+    * 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 Assignee for the PR.


and an Assignee

joeyaiello · 2017-02-21T23:44:17Z

.github/CONTRIBUTING.md

+      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 Assignee for the PR.
+    If no Assignee is chosen, one of Maintainers shall be assigned to it.


one of the Maintainers...

joeyaiello · 2017-02-21T23:44:40Z

.github/CONTRIBUTING.md

+      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 Assignee for the PR.
+    If no Assignee is chosen, one of Maintainers shall be assigned to it.
+    If no Reviewer is chosen, Assignee shall choose Reviewer(s) as appropriate.


is chosen, the Assignee

joeyaiello · 2017-02-21T23:45:38Z

.github/CONTRIBUTING.md

+    If no Assignee is chosen, one of Maintainers shall be assigned to it.
+    If no Reviewer is chosen, 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 Assignee of the PR.


to choose Reviewer(s) and an Assignee for the PR

joeyaiello · 2017-02-21T23:46:02Z

.github/CONTRIBUTING.md

+    * If an Author is a [PowerShell Team](https://github.com/orgs/PowerShell/people) member,
+      then the Author **is required** to choose Reviewer(s) and Assignee of 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 Assignee as appropriate.


by the Assignee as appropriate

daxian-dbw · 2017-02-22T00:22:17Z

@joeyaiello Thanks for the review! I have addressed your comments. Please take another look.

joeyaiello

LGTM!

SteveL-MSFT · 2017-02-22T19:48:05Z

.github/CONTRIBUTING.md

+    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,


Collaborator should have a capital 'C' as it's a specific group

iSazonov · 2017-02-22T20:15:14Z

I can not to choose Reviewer(s) and Assignee for my PR 😕

daxian-dbw · 2017-02-22T21:35:21Z

@iSazonov hmm, I didn't know about that ... so I guess then all non-powershell-team users cannot choose Reviewer and Assignee? @TravisEz13 do you happen to know if that's the case?

@SteveL-MSFT, the collaborator has been changed to Collaborator.

TravisEz13 · 2017-02-22T23:41:42Z

@daxian-dbw I just submitted a review to PSScriptAnalyzer and I cannot even assign a reviewer there.

iSazonov · 2017-02-23T15:28:34Z

@daxian-dbw Yes, it is. Personally for me it is not a problem. I can always make a ping by "/cc".

Define roles and their responsibilities in a PR

a5901da

msftclas added the cla-not-required label Feb 17, 2017

daxian-dbw requested review from HemantMahawar, SteveL-MSFT, TravisEz13, andyleejordan, joeyaiello, lzybkr, mirichmo and vors February 17, 2017 21:25

daxian-dbw assigned TravisEz13 Feb 17, 2017

TravisEz13 requested changes Feb 17, 2017

View reviewed changes

Fix grammar issues in the text change

38b03e8

PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 17, 2017

TravisEz13 approved these changes Feb 17, 2017

View reviewed changes

Fix some more grammar issues

bc7c82a

mirichmo suggested changes Feb 18, 2017

View reviewed changes

one more fix

8ae5c75

vors approved these changes Feb 20, 2017

View reviewed changes

andyleejordan approved these changes Feb 21, 2017

View reviewed changes

SteveL-MSFT requested changes Feb 21, 2017

View reviewed changes

Address review comments

c9f1bf5

SteveL-MSFT approved these changes Feb 21, 2017

View reviewed changes

mirichmo approved these changes Feb 21, 2017

View reviewed changes

joeyaiello reviewed Feb 21, 2017

View reviewed changes

Address Joey's comments

a04ca85

joeyaiello approved these changes Feb 22, 2017

View reviewed changes

SteveL-MSFT requested changes Feb 22, 2017

View reviewed changes

Caps 'C' in 'Collaborator'

e66f55a

lzybkr approved these changes Feb 22, 2017

View reviewed changes

lzybkr merged commit a1edf8f into PowerShell:master Feb 23, 2017

daxian-dbw deleted the pr branch February 23, 2017 22:07

iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017

Define roles and their responsibilities in a PR #3168

Define roles and their responsibilities in a PR #3168

Uh oh!

Conversation

daxian-dbw commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 20, 2017

Uh oh!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleejordan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 17, 2017 •

edited

Loading

daxian-dbw Feb 20, 2017 •

edited

Loading