From aa1ca270f70de4e7ca3a9a972d8c59437b60eec2 Mon Sep 17 00:00:00 2001 From: Chris Dent Date: Tue, 26 Mar 2019 16:02:01 +0000 Subject: [PATCH] Fill in reviewing section of contributing.rst This provides some links to more info about reviewing, and some guidelines on how to be a good reviewer, with special considerations for cores. Not included is information on how a patch submitter should hassle someone to get attention. With the low volume of patches that go through placement, that should not be something the submitter should have to care about. It's the job of regular reviewers to be aware of new stuff. The guideline about latency covers that. Change-Id: I61cf791c956a75304b87ffb4a16a8252fc36800b --- doc/source/contributor/contributing.rst | 61 +++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/doc/source/contributor/contributing.rst b/doc/source/contributor/contributing.rst index abd37bf12..5d4a38a67 100644 --- a/doc/source/contributor/contributing.rst +++ b/doc/source/contributor/contributing.rst @@ -80,6 +80,63 @@ gerrit. Adding ``Task:`` will update the identified task to indicate it is in a Reviewing Code -------------- +Like other OpenStack projects, Placement uses `gerrit`_ to facilitate peer code +review. It is hoped and expected that anyone who would like to commit code to +the Placement project will also spend time reviewing code for the sake of the +common good. The more people reviewing, the more code that will eventually +merge. + +See `How to Review Changes the OpenStack Way`_ for an overview of the review +and voting process. + +There is a small group of people within the Placement team called `core +reviewers`_. These are people who are empowered to signal (via the ``+2`` vote) +that code is of a suitable standard to be merged and is aligned with the +current goals of the project. Core reviewers are regularly selected from all +active reviewers based on the quantity and quality of their reviews and +demonstrated understanding of the Placement code and goals of the project. + +The point of review is to evolve potentially useful code to merged working code +that is aligned with the standards of style, testing, and correctness that we +share as group. It is not for creating perfect code. Review should always be +`constructive`_, encouraging, and friendly. People who contribute code are +doing the project a favor, make it feel that way. + +Some guidelines that reviewers and patch submitters should be aware of: + +* It is very important that a new patch set gets some form of review as soon as + possible, even if only to say "we've seen this". Latency in the review + process has been identified as hugely discouraging for new and experienced + contributors alike. +* Follow up changes, to fix minor problems identified during review, are + encouraged. We want to keep things moving. +* As a reviewer, remember that not all patch submitters will know these + guidelines. If it seems they don't, point them here and be patient in the + meantime. +* Gerrit can be good for code review, but is often not a great environment for + having a discussion that is struggling to resolve to a decision. Move + discussion to the mailing list sooner rather than later. Add a link to the + thread in the `list archive`_ to the review. +* If the CI system is throwing up random failures in test runs, you should + endeavor whenever possible to investigate, not simply ``recheck``. A flakey + gate is an indication that OpenStack is not robust and at the root of all + this, making OpenStack work well is what we are doing. + + +Special Considerations For Core Reviewers +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Core reviewers have special powers. With great power comes great responsibility +and thus being held to a standard. As a core reviewer, your job is to enable +other people to contribute good code. Under ideal conditions it is more +important to be reviewing other people's code and bugs and fixing bugs than it +is to be writing your own features. Frequently conditions will not be ideal, +but strive to enable others. + +When there are open questions that need to be resolved, try to prefer the +`openstack-discuss`_ list over IRC so that anyone can be involved according +to their own schedules and input from unexpected sources can be available. + Writing Code ------------ @@ -92,6 +149,10 @@ New Features .. _Project Team Guide: https://docs.openstack.org/project-team-guide/ .. _Developer's Guide: https://docs.openstack.org/infra/manual/developers.html .. _openstack-discuss: http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-discuss +.. _list archive: http://lists.openstack.org/pipermail/openstack-discuss/ .. _StoryBoard: https://storyboard.openstack.org/#!/project/openstack/placement .. _new bug: https://storyboard.openstack.org/#!/worklist/580 .. _gerrit: http://review.openstack.org/ +.. _How to Review Changes the OpenStack Way: https://docs.openstack.org/project-team-guide/review-the-openstack-way.html +.. _core reviewers: https://review.openstack.org/#/admin/groups/1936,members +.. _constructive: https://governance.openstack.org/tc/reference/principles.html#we-value-constructive-peer-review