diff options
Diffstat (limited to 'doc/rtd/topics/code_review.rst')
| -rw-r--r-- | doc/rtd/topics/code_review.rst | 256 |
1 files changed, 256 insertions, 0 deletions
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. |