"How to do reviews and PRs" document needed

[castrojo]

Part of #1413

We have https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#best-practices-for-faster-reviews for the person submitting the reviews, but we are missing a document for people doing reviews. We need to collate some tips and tricks from reviewers from across the project.

[castrojo]

/sig contributor-experience

[chrislovecnm]

/assign @parispittman
/assign

[chrislovecnm]

/cc @thockin

We talked about this Tim @ kubecon :) Dragging you into it kicking a screaming. NEED your help!!

/cc @justinsb

Since you are opinionated on PR reviews, need your help.

[tpepper]

I like to use the docs like https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#best-practices-for-faster-reviews as a starting point for reviewers. The reviewer and reviewee docs should be symmetric.

A lot of the existing doc is around mechanical "how" parts. For the philosophical parts I like to refer people to http://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ but some of this varies with project and the local k8s culture needs to be documented by local gurus.

I really like this https://www.youtube.com/watch?feature=player_embedded&v=fMeH7wqOwXA#! old talk on reviewer/approver/maintainer issues because it has a senior person describing their review process and workload. Reviewers need to be skeptical, have a broad view for technical debt and potential collateral damage. Has the change been sufficiently justified and tested? I also like that it comes from a time when that project was suddenly experiencing a huge spurt of growth (arm & arm64 and android drivers) and figuring out how to incentivize large out-of-tree code contributors to stay closer to upstream, which parallels @thockin's discussion about the kernel vs distro split concept.

[chrislovecnm]

@parispittman mentioned this on slack https://twitter.com/math_rachel/status/939968380202762240

[castrojo]

Notes from call with guin: this should live in community/contributor/guide/ and we should move and scrub the "best practices for faster reviews" so that the pair of docs are together.

[parispittman]

Created this doc - appreciate collaboration. This will be used as a resource when mentoring new reviewers.

"Code Reviews the Kuberetes Way"

@castrojo @guineveresaenger

[tpepper]

I've dropped some related doc links in the google doc.

[castrojo]

More reference material to check out: https://github.com/kubernetes/charts/blob/master/REVIEW_GUIDELINES.md

[castrojo]

See also: https://docs.google.com/document/d/1WVobATuodV0T3F22CXhsdYpOk5K7D-uKO9j-0QUuLdU/edit#heading=h.2oi459gj6360

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[guineveresaenger]

/remove-lifecycle stale

[guineveresaenger]

Current status summary of this issue:

• Reviews from the PR author's perspective are covered well as per Cleanup and move to /contributors/guide of pull requests doc #1761 .
• "How to review" still deeds doing; this is a gig for an experienced contributor/reviewer. For completion:

1. There is some material in Move assigned reviewers snippet from collab.md somewhere else. #1645 that should go into the reviewer guidelines
2. A good location for this is either a new subsection of https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md
   or quite possibly a doc of its own, in the spirit of https://github.com/kubernetes/community/blob/master/contributors/devel/collab.md but it should live in contributors/guide not contributors/devel.
3. https://github.com/kubernetes/community/blob/master/contributors/devel/collab.md needs to be moved and possibly renamed or sunsetted - the goal should be that it's in a logical, easy to find place, with a comprehensive title ("Code Review The Kubernetes Way" being a top contender).

[guineveresaenger]

/help

[guineveresaenger]

/remove-help

Apologies, this should be done by an experienced contributor.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[idealhack]

/remove-lifecycle stale
/lifecycle frozen

[vishakhanihore]

/assign