Add how to code review a change 38/9138/3
authorJessica Wagantall <jwagantall@linuxfoundation.org>
Fri, 2 Mar 2018 07:41:35 +0000 (23:41 -0800)
committerThanh Ha <thanh.ha@linuxfoundation.org>
Fri, 2 Mar 2018 18:43:15 +0000 (13:43 -0500)
Add documentation on how to code review:
- Pre-review
- Review
- Merge

Issue-ID: RELENG-559
Change-Id: I527e7aafd0b9a444fc6ac5593a96567bf0015598
Signed-off-by: Jessica Wagantall <jwagantall@linuxfoundation.org>
docs/_static/gerrit-code-review-votes.png [new file with mode: 0644]
docs/_static/gerrit-diff-menu.png [new file with mode: 0644]
docs/_static/gerrit-inline-feedback.png [new file with mode: 0644]
docs/_static/gerrit-patch-update-history.png [new file with mode: 0644]
docs/_static/gerrit-publish-button.png [new file with mode: 0644]
docs/_static/gerrit-reviewers-interface.png [new file with mode: 0644]
docs/_static/gerrit-voting-interface.png [new file with mode: 0644]
docs/_static/gerrit-wide-view.png [new file with mode: 0644]
docs/gerrit.rst

diff --git a/docs/_static/gerrit-code-review-votes.png b/docs/_static/gerrit-code-review-votes.png
new file mode 100644 (file)
index 0000000..1cdfbfc
Binary files /dev/null and b/docs/_static/gerrit-code-review-votes.png differ
diff --git a/docs/_static/gerrit-diff-menu.png b/docs/_static/gerrit-diff-menu.png
new file mode 100644 (file)
index 0000000..c800ef1
Binary files /dev/null and b/docs/_static/gerrit-diff-menu.png differ
diff --git a/docs/_static/gerrit-inline-feedback.png b/docs/_static/gerrit-inline-feedback.png
new file mode 100644 (file)
index 0000000..abda086
Binary files /dev/null and b/docs/_static/gerrit-inline-feedback.png differ
diff --git a/docs/_static/gerrit-patch-update-history.png b/docs/_static/gerrit-patch-update-history.png
new file mode 100644 (file)
index 0000000..71d6be6
Binary files /dev/null and b/docs/_static/gerrit-patch-update-history.png differ
diff --git a/docs/_static/gerrit-publish-button.png b/docs/_static/gerrit-publish-button.png
new file mode 100644 (file)
index 0000000..60e88d0
Binary files /dev/null and b/docs/_static/gerrit-publish-button.png differ
diff --git a/docs/_static/gerrit-reviewers-interface.png b/docs/_static/gerrit-reviewers-interface.png
new file mode 100644 (file)
index 0000000..4086fad
Binary files /dev/null and b/docs/_static/gerrit-reviewers-interface.png differ
diff --git a/docs/_static/gerrit-voting-interface.png b/docs/_static/gerrit-voting-interface.png
new file mode 100644 (file)
index 0000000..7bd0090
Binary files /dev/null and b/docs/_static/gerrit-voting-interface.png differ
diff --git a/docs/_static/gerrit-wide-view.png b/docs/_static/gerrit-wide-view.png
new file mode 100644 (file)
index 0000000..df6b0ed
Binary files /dev/null and b/docs/_static/gerrit-wide-view.png differ
index bc96302..6a78f20 100644 (file)
@@ -248,6 +248,128 @@ You will receive 2 emails from Gerrit Code Review: the first indicating that a b
 to incorporate your changes has started; and the second indicating the creation of the
 build.
 
+Code Review
+===========
+
+All contributions to Git repositories use Gerrit for code review.
+
+The code review process provides constructive feedback about a proposed change.
+Committers and interested contributors will review the change, give their feedback,
+propose revisions and work with the change author through iterations of the patch until
+it’s ready for merging.
+
+Managing and providing feedback for the change happens via Gerrit web UI.
+
+.. figure:: _static/gerrit-wide-view.png
+
+   Gerrit wide view.
+
+Pre-review
+----------
+
+Change authors will want to push changes to Gerrit before they are actually
+ready for review. This is an encoraged good practice. It has been the
+experience of experienced community members that pushing often tends to reduce
+the amount of work and ensures speedy code reviews.
+
+.. note::
+
+    This is not required and in some projects, not encouraged, but the general idea
+    of making sure patches are ready for review when submitted is a good one.
+
+.. note::
+
+    While in draft state, Gerrit triggers, e.g., verify Jenkins jobs, won’t run
+    by default. You can trigger them despite it being a draft by adding
+    "Jenkins CI" (or the corresponding Jenkins automation account) as a
+    reviewer. You may need to do a recheck by replying with a comment
+    containing ``recheck`` to trigger the jobs after adding the reviewer.
+
+To mark an uploaded change as not ready for attention by committers and interested
+contributors (in order of preference), either mark the Gerrit a draft (by adding
+a ``-D`` to your ``git review`` command), vote -1 on it yourself or edit the commit
+message with "WIP" ("Work in Progress").
+
+Do not add committers to the Reviewers list for a change while in the pre-review
+state, as it adds noise to their review queue.
+
+Review
+------
+
+Once an author wants a change reviewed, they need to take some actions to put it on
+the radar of the committers.
+
+If the change it's a draft, you'll need to publish it. Do this from the Gerrit web UI.
+
+.. figure:: _static/gerrit-publish-button.png
+
+   Gerrit Web UI button to publish a draft change.
+
+Remove your -1 vote if you've marked it with one. If you think the patch is ready for
+merge, vote +1. If there is not an automated job to test your change and vote +1/-1
+for Verified, you will need to do as much testing yourself as possible and then manually
+vote +1 to Verified. You can also vote +1 for Verified if you have done testing in
+addition to any automated tests. Describing the testing you did or did not do is
+typically helpful.
+
+.. figure:: _static/gerrit-voting-interface.png
+
+   Gerrit voting interface, exposed by the Reply button.
+
+Once the change gets published and you have voted for merging, add the people who
+need to review/merge the change to the Gerrit Reviewers list. The auto-complete for
+this Gerrit UI field is somewhat flaky, but typing the full name from the start
+typically works.
+
+.. figure:: _static/gerrit-reviewers-interface.png
+
+   Gerrit Reviewers list with Int/Pack committers added
+
+Reviewers will give feedback via Gerrit comments or inline against the diff.
+
+.. figure:: _static/gerrit-inline-feedback.png
+
+   Gerrit inline feedback about a typo
+
+Updated versions of the proposed change get pushed as new patchesets to the same
+Gerrit, either by the original submitter or other contributors. Amending proposed changes
+owned by others while reviewing may be more efficient than documenting the problem, -1ing,
+waiting for the original submitter to make the changes, re-reviewing and merging.
+
+Download changes for local manipulation and re-uploaded updates via git-review.
+
+See `Update an existing patch`_ above. Once you have re-uploaded the patch the Gerrit web
+UI for the proposed change will reflect the new patcheset.
+
+.. figure:: _static/gerrit-patch-update-history.png
+
+   Gerrit history showing a patch update
+
+Reviewers will use the diff between the last time they gave review and the current patchset
+to understand updates, speeding the code review process.
+
+.. figure:: _static/gerrit-diff-menu.png
+
+   Gerrit diff menu
+
+Iterative feedback continues until reaching consensus (typically: all active reviewers +1/+2
+and no -1s nor -2s), at least one committer +2s and a committer merges the change.
+
+.. figure:: _static/gerrit-code-review-votes.png
+
+   Gerrit code review votes
+
+Merge
+-----
+
+Once a patch has gotten a +2 from a committer and they have clicked the submit button the
+project's merge job should run and publish the project's artifacts to Nexus. Once completed,
+other projects will be able to see the results of that patch.
+
+This is important when merging dependent patches across projects. You will need to wait
+for the merge job to run on one patch before any patches in other projects depending on
+it will successful verify.
+
 Setting up Gerrit
 =================