Merge "[DevRef] Add code review guideline"
This commit is contained in:
commit
d144bf7593
|
@ -1,3 +1,5 @@
|
||||||
|
.. _code-reviews-with-gerrit:
|
||||||
|
|
||||||
Code Reviews with Gerrit
|
Code Reviews with Gerrit
|
||||||
========================
|
========================
|
||||||
|
|
||||||
|
|
|
@ -58,6 +58,7 @@ Other Resources
|
||||||
|
|
||||||
launchpad
|
launchpad
|
||||||
gerrit
|
gerrit
|
||||||
|
manila-review-policy
|
||||||
|
|
||||||
API Reference
|
API Reference
|
||||||
-------------
|
-------------
|
||||||
|
|
|
@ -0,0 +1,107 @@
|
||||||
|
.. _manila-review-policy:
|
||||||
|
|
||||||
|
Manila team code review policy
|
||||||
|
==============================
|
||||||
|
|
||||||
|
Peer code review and the OpenStack Way
|
||||||
|
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||||
|
|
||||||
|
Manila adheres to the `OpenStack code review policy and guidelines
|
||||||
|
<https://docs.openstack.org/infra/manual/developers.html#peer-review>`_.
|
||||||
|
Similar to other projects hosted on `git.openstack.org <http://git.openstack
|
||||||
|
.org/cgit>`_, all of manila's code is curated and maintained by a small
|
||||||
|
group of individuals called the "core team". The `primary core team
|
||||||
|
<https://review.openstack.org/#/admin/groups/213,members>`_
|
||||||
|
consists of members from diverse affiliations. There are special core teams
|
||||||
|
such as the `manila release core team <https://review.openstack
|
||||||
|
.org/#/admin/groups/215,members>`_ and the `manila stable maintenance core
|
||||||
|
team <https://review.openstack.org/#/admin/groups/1099,members>`_ that
|
||||||
|
have specific roles as the names suggest.
|
||||||
|
|
||||||
|
To make a code change in openstack/manila or any of the associated code
|
||||||
|
repositories (openstack/manila-image-elements, openstack/manila-specs,
|
||||||
|
openstack/manila-tempest-plugin, openstack/manila-test-image,
|
||||||
|
openstack/manila-ui and openstack/python-manilaclient), contributors need to
|
||||||
|
follow the :ref:`Code Submission Process <code-reviews-with-gerrit>` and
|
||||||
|
upload their code on the `OpenStack Gerrit <https://review.openstack.org>`_
|
||||||
|
website. They can then seek reviews by adding individual members of the
|
||||||
|
`manila core team <https://review.openstack.org/#/admin/groups/213,
|
||||||
|
members>`_ or alert the entire core team by inviting the Gerrit group
|
||||||
|
"manila-core" to the review. Anyone with a membership to the OpenStack
|
||||||
|
Gerrit system may review the code change. However, only the core team can
|
||||||
|
accept and merge the code change. Reviews from contributors outside the core
|
||||||
|
team are encouraged. Reviewing code meticulously and often is a
|
||||||
|
pre-requisite for contributors aspiring to join the core reviewer team.
|
||||||
|
|
||||||
|
One or more core reviewers will take cognizance of the contribution and
|
||||||
|
provide feedback, or accept the code. For the submission to be accepted, it
|
||||||
|
will need a minimum of one Code-Review:+2 and one Workflow:+1 votes, along
|
||||||
|
with getting a Verified:+1 vote from the CI system. If no core reviewer pays
|
||||||
|
attention to a code submission, feel free to remind the team on the
|
||||||
|
#openstack-manila IRC channel on irc.freenode.com. [#]_ [#]_
|
||||||
|
|
||||||
|
Core code review guidelines
|
||||||
|
~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||||
|
|
||||||
|
By convention rather than rule, we require that a minimum of two code
|
||||||
|
reviewers provide a Code-Review:+2 vote on each code submission before it is
|
||||||
|
given a Workflow:+1 vote. Having two core reviewers approve a change adds
|
||||||
|
diverse perspective, and is extremely valuable in case of:
|
||||||
|
|
||||||
|
- Feature changes in the manila service stack
|
||||||
|
- Changes to configuration options
|
||||||
|
- Addition of new tests or significant test bug-fixes in manila-tempest-plugin
|
||||||
|
- New features to manila-ui, manila-test-image, manila-image-elements
|
||||||
|
- Bug fixes
|
||||||
|
|
||||||
|
Trivial changes
|
||||||
|
---------------
|
||||||
|
Trivial changes are:
|
||||||
|
|
||||||
|
- Continuous Integration (CI) system break-fixes that are simple,
|
||||||
|
i.e.:
|
||||||
|
|
||||||
|
- No job or test is being deleted
|
||||||
|
- Change does not break third-party CI
|
||||||
|
|
||||||
|
- Documentation changes, especially typographical fixes and grammar
|
||||||
|
corrections.
|
||||||
|
- Automated changes generated by tooling - translations, lower-requirements
|
||||||
|
changes, etc.
|
||||||
|
|
||||||
|
We do not need two core reviewers to approve trivial changes.
|
||||||
|
|
||||||
|
Affiliation of core reviewers
|
||||||
|
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||||
|
Previously, the manila core team informally enforced a code review
|
||||||
|
convention that each code change be reviewed and merged by
|
||||||
|
reviewers of different affiliations. This was followed because the
|
||||||
|
OpenStack Technical Committee used the diversity of
|
||||||
|
affiliation of the core reviewer team as a metric for maturity of the
|
||||||
|
project. However, since the Rocky release cycle, the TC has changed its view
|
||||||
|
on the subject [#]_ [#]_. We believe this is a step in the right
|
||||||
|
direction.
|
||||||
|
|
||||||
|
While there is no strict requirement that two core reviewers accepting
|
||||||
|
a code change have different affiliations. Other things being equal, we will
|
||||||
|
continue to informally encourage organizational diversity by having core
|
||||||
|
reviewers from different organizations. Core reviewers have the professional
|
||||||
|
responsibility of avoiding conflicts of interest.
|
||||||
|
|
||||||
|
Vendor code and review
|
||||||
|
~~~~~~~~~~~~~~~~~~~~~~
|
||||||
|
All code in the manila repositories is open-source and anyone can submit
|
||||||
|
changes to these repositories as long as they seek to improve the code base.
|
||||||
|
Manila supports over 30 vendor storage systems, and many of these vendors
|
||||||
|
participate in the development and maintenance of their drivers. To the
|
||||||
|
extent possible, core reviewers will seek out driver maintainer feedback on
|
||||||
|
code changes pertaining to vendor integrations.
|
||||||
|
|
||||||
|
|
||||||
|
References
|
||||||
|
~~~~~~~~~~
|
||||||
|
|
||||||
|
.. [#] Getting started with IRC: https://docs.openstack.org/contributors/common/irc.html
|
||||||
|
.. [#] IRC guidelines: https://docs.openstack.org/infra/manual/irc.html
|
||||||
|
.. [#] TC Report 18-28: https://anticdent.org/tc-report-18-28.html
|
||||||
|
.. [#] TC vote to remove team diversity tags: https://review.openstack.org/#/c/579870/
|
Loading…
Reference in New Issue