doc: introduce Code Review Process documentation (#160)

2 files changed, 257 insertions, 0 deletions

diff --git a/doc/rtd/index.rst b/doc/rtd/index.rst
index 5d90c131..0015e35a 100644
--- a/doc/rtd/index.rst
+++ b/doc/rtd/index.rst
@@ -68,6 +68,7 @@ Having trouble? We would like to help!
    :caption: Development
 
    topics/hacking.rst
+   topics/code_review.rst
    topics/security.rst
    topics/debugging.rst
    topics/logging.rst
diff --git a/doc/rtd/topics/code_review.rst b/doc/rtd/topics/code_review.rst
new file mode 100644
index 00000000..68c10405
--- /dev/null
+++ b/doc/rtd/topics/code_review.rst
@@ -0,0 +1,256 @@
+*******************
+Code Review Process
+*******************
+
+In order to manage incoming pull requests effectively, and provide
+timely feedback and/or acceptance this document serves as a guideline
+for the review process and outlines the expectations for those
+submitting code to the project as well as those reviewing the code.
+Code is reviewed for acceptance by at least one core team member (later
+referred to as committers), but comments and suggestions from others
+are encouraged and welcome.
+
+The process is intended to provide timely and actionable feedback for
+any submission.
+
+Asking For Help
+===============
+
+cloud-init contributors, potential contributors, community members and
+users are encouraged to ask for any help that they need.  If you have
+questions about the code review process, or at any point during the
+code review process, these are the available avenues:
+
+* if you have an open Pull Request, comment on that pull request
+* join the ``#cloud-init`` channel on the Freenode IRC network and ask
+  away
+* send an email to the cloud-init mailing list,
+  cloud-init@lists.launchpad.net
+
+These are listed in rough order of preference, but use whichever of
+them you are most comfortable with.
+
+Goals
+=====
+
+This process has the following goals:
+
+* Ensure code reviews occur in a timely fashion and provide actionable
+  feedback if changes are desired.
+* Ensure the minimization of ancillary problems to increase the
+  efficiency for those reviewing the submitted code
+
+Role Definitions
+================
+
+Any code review process will have (at least) two involved parties.  For
+our purposes, these parties are referred to as **Proposer** and
+**Reviewer**.  (We also have the **Committer** role which is a special
+case of the **Reviewer** role.)  The terms are defined here (and the
+use of the singular form is not meant to imply that they refer to a
+single person):
+
+Proposer
+   The person proposing a pull request (hereafter known as a PR).
+
+Reviewer
+   A person who is reviewing a PR.
+
+Committer
+   A cloud-init core developer (i.e. a person who has permission to
+   merge PRs into master).
+
+Prerequisites For Landing Pull Requests
+=======================================
+
+Before a PR can be landed into master, the following conditions *must*
+be met:
+
+* the CLA has been signed by the **Proposer** (or is covered by an
+  entity-level CLA signature)
+* all required status checks are passing
+* at least one "Approve" review from a **Committer**
+* no "Request changes" reviews from any **Committer**
+
+The following conditions *should* be met:
+
+* any Python functions/methods/classes have docstrings added/updated
+* any changes to config module behaviour are captured in the
+  documentation of the config module
+* any Python code added has corresponding unit tests
+* no "Request changes" reviews from any **Reviewer**
+
+These conditions can be relaxed at the discretion of the
+**Committers** on a case-by-case basis.  Generally, for accountability,
+this should not be the decision of a single **Committer**, and the
+decision should be documented in comments on the PR.
+
+(To take a specific example, the ``cc_phone_home`` module had no tests
+at the time `PR #237
+<https://github.com/canonical/cloud-init/pull/237>`_ was submitted, so
+the **Proposer** was not expected to write a full set of tests for
+their minor modification, but they were expected to update the config
+module docs.)
+
+Non-Committer Reviews
+=====================
+
+Reviews from non-**Committers** are *always* welcome.  Please feel
+empowered to review PRs and leave your thoughts and comments on any
+submitted PRs, regardless of the **Proposer**.
+
+Much of the below process is written in terms of the **Committers**.
+This is not intended to reflect that reviews should only come from that
+group, but acknowledges that we are ultimately responsible for
+maintaining the standards of the codebase.  It would be entirely
+reasonable (and very welcome) for a **Reviewer** to only examine part
+of a PR, but it would not be appropriate for a **Committer** to merge a
+PR without full scrutiny.
+
+Opening Phase
+=============
+
+In this phase, the **Proposer** is responsible for opening a pull
+request and meeting the prerequisites laid out above.
+
+If they need help understanding the prerequisites, or help meeting the
+prerequisites, then they can (and should!) ask for help.  See the
+:ref:`Asking For Help` section above for the ways to do that.
+
+These are the steps that comprise the opening phase:
+
+1. The **Proposer** opens PR
+
+2. CI runs automatically, and if
+
+   CI fails
+      The **Proposer** is expected to fix CI failures.  If the
+      **Proposer** doesn't understand the nature of the failures they
+      are seeing, they should comment in the PR to request assistance,
+      or use another way of :ref:`Asking For Help`.
+
+      (Note that if assistance is not requested, the **Committers**
+      will assume that the **Proposer** is working on addressing the
+      failures themselves.  If you require assistance, please do ask
+      for help!)
+
+   CI passes
+      Move on to the :ref:`Review phase`.
+
+Review Phase
+============
+
+In this phase, the **Proposer** and the **Reviewers** will iterate
+together to, hopefully, get the PR merged into the cloud-init codebase.
+There are three potential outcomes: merged, rejected permanently, and
+temporarily closed.  (The first two are covered in this section; see
+:ref:`Inactive Pull Requests` for details about temporary closure.)
+
+(In the below, when the verbs "merge" or "squash merge" are used, they
+should be understood to mean "squash merged using the GitHub UI", which
+is the only way that changes can land in cloud-init's master branch.)
+
+These are the steps that comprise the review phase:
+
+1. **The Committers** assign a **Committer** to the PR
+
+   This **Committer** is expected to shepherd the PR to completion (and
+   merge it, if that is the outcome reached).  This means that they
+   will perform an initial review, and monitor the PR to ensure that
+   the **Proposer** is receiving any assistance that they require.  The
+   **Committers** will perform this assignment on a daily basis.
+
+   This assignment is intended to ensure that the **Proposer** has a
+   clear point of contact with a cloud-init core developer, and that
+   they get timely feedback after submitting a PR.  It *is not*
+   intended to preclude reviews from any other **Reviewers**, nor to
+   imply that the **Committer** has ownership over the review process.
+
+   The assigned **Committer** may choose to delegate the code review of
+   a PR to another **Reviewer** if they think that they would be better
+   suited.
+
+   (Note that, in GitHub terms, this is setting an Assignee, not
+   requesting a review.)
+
+2. That **Committer** performs an initial review of the PR, resulting
+   in one of the following:
+
+   Approve
+     If the submitted PR meets all of the :ref:`Prerequisites for
+     Landing Pull Requests` and passes code review, then the
+     **Committer** will squash merge immediately.
+
+     There may be circumstances where a PR should not be merged
+     immediately.  The ``wip`` label will be applied to PRs for which
+     this is true.  Only **Committers** are able to apply labels to
+     PRs, so anyone who believes that this label should be applied to a
+     PR should request its application in a comment on the PR.
+
+     The review process is **DONE**.
+
+   Approve (with nits)
+     If the **Proposer** submits their PR with "Allow edits from
+     maintainer" enabled, and the only changes the **Committer**
+     requests are minor "nits", the **Committer** can push fixes for
+     those nits and *immediately* squash merge.  If the **Committer**
+     does not wish to fix these nits but believes they should block a
+     straight-up Approve, then their review should be "Needs Changes"
+     instead.
+
+     A nit is understood to be something like a minor style issue or a
+     spelling error, generally confined to a single line of code.
+
+     If a **Committer** is unsure as to whether their requested change
+     is a nit, they should not treat it as a nit.
+
+     (If a **Proposer** wants to opt-out of this, then they should
+     uncheck "Allow edits from maintainer" when submitting their PR.)
+
+     The review process is **DONE**.
+
+   Outright rejection
+     The **Committer** will close the PR, with useful messaging for the
+     **Proposer** as to why this has happened.
+
+     This is reserved for cases where the proposed change is completely
+     unfit for landing, and there is no reasonable path forward.  This
+     should only be used sparingly, as there are very few cases where
+     proposals are completely unfit.
+
+     If a different approach to the same problem is planned, it should
+     be submitted as a separate PR.  The **Committer** should include
+     this information in their message when the PR is closed.
+
+     The review process is **DONE**.
+
+   Needs Changes
+     The **Committer** will give the **Proposer** a clear idea of what
+     is required for an Approve vote or, for more complex PRs, what the
+     next steps towards an Approve vote are.
+
+     The **Proposer** will ask questions if they don't understand, or
+     disagree with, the **Committer**'s review comments.
+
+     Once consensus has been reached, the **Proposer** will address the
+     review comments.
+
+     Once the review comments are addressed (as well as, potentially,
+     in the interim), CI will run.  If CI fails, the **Proposer** is
+     expected to fix CI failures.  If CI passes, the **Proposer**
+     should indicate that the PR is ready for re-review (by @ing the
+     assigned reviewer), effectively moving back to the start of this
+     section.
+
+Inactive Pull Requests
+======================
+
+PRs will be temporarily closed if they have been waiting on
+**Proposer** action for a certain amount of time without activity.  A
+PR will be marked as stale (with an explanatory comment) after 14 days
+of inactivity.  It will be closed after a further 7 days of inactivity.
+
+These closes are not considered permanent, and the closing message
+should reflect this for the **Proposer**. However, if a PR is reopened,
+it should effectively enter the :ref:`Opening phase` again, as it may
+need some work done to get CI passing again.