Define standard guidance on pull request reviews for charters and RFCs

[brianraymor]

Noted on the November 15 DCP PM call:

Tony raised the lack of clear etiquette with respect to the charter and RFC process with respect to how to comment/reject/approve using the standard git tools. Trevor confirmed using these flags worked well for the Data Portal and Data Browser charters

• Update charter process
• Update RFC process

[lauraclarke]

A quick thought on this

The github review process has three statuses

Comment: Submit general feedback without explicitly approving the changes or requesting additional changes.
Approve: Submit feedback and approve merging the changes proposed in the pull request.
Request changes: Submit feedback that must be addressed before the pull request can be merged.

I propose that people don't use the comment option at all.

If you don't think the text should change but have some thoughts about the outcome/future direction/missing bits approve and leave a comment with your approval.

If you have comments which you feel need review/resolution before the charter/rfc is approved use the inline comment tool to point to the bits you feel need to change and then mark as rejected.

The shepherd/author is then responsible for chasing anyone who has rejected the RFC/charter to switch to approve before moving forward and can read and if needed take action on comments left with the approved status but isn't obliged too.

Do we also want a policy about resolving inline comments associated with particular pieces of text, there is an option to resolve a particular conversation? should the instigator or shepherd be responsible for resolving each individual conversation or not?

[brianraymor]

I completely agree with I propose that people don't use the comment option at all.

I'm more on the fence about the difference between Approve and Request Changes. If I have thoughts about missing bits, then it seems like I would want that addressed before the charter is approved. I would suggest that if a review consists only of minor editorial changes (like spelling), and/or suggestions then Approve; otherwise, Request Changes.

I also agree with The shepherd/author is then responsible for chasing anyone who has rejected the RFC/charter to switch to approve before moving forward ... - the comments need to be addressed but not necessarily accepted.

I'd prefer to avoid a policy requiring the resolution of inline comments.

[brianraymor]

@lauraclarke and @tburdett - I'd propose the following language for charters (similar language to be adopted for RFCs (with the approval burden falling on the shepherd):

Reviewers:

Community members review and comment on a charter pull request with a pull request review. It has three possible options: Comment, Approve, or Request changes.

For charter reviews, Comment SHOULD NOT be used.

If a review consists of minor editorial changes (such as spelling) and/or optional recommendations then the reviewer SHOULD submit Approve; otherwise, the reviewer SHOULD submit Request changes.

Authors:

Revise the charter pull request in response to feedback and push new commits

In the case of Request changes, the Author MUST ensure the reviewer updates Request changes to Approve before the charter can progress.

When all issues are addressed and any required Science reviews are complete: ...