diff options
Diffstat (limited to 'doc/rtd/development/code_review.rst')
| -rw-r--r-- | doc/rtd/development/code_review.rst | 261 |
1 files changed, 261 insertions, 0 deletions
diff --git a/doc/rtd/development/code_review.rst b/doc/rtd/development/code_review.rst new file mode 100644 index 00000000..5684a509 --- /dev/null +++ b/doc/rtd/development/code_review.rst @@ -0,0 +1,261 @@ +.. _code_review_process: + +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. It 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 Libera IRC`_ network, +* send an email to the cloud-init mailing list: :: + + cloud-init@lists.launchpad.net + +These are listed in order of preference, but please use whichever of them you +are most comfortable with. + +Goals +===== + +This process has the following goals: + +* To ensure code reviews occur in a timely fashion and provide actionable + feedback if changes are desired. +* To ensure the minimisation 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 ``main``). + +Prerequisites for landing pull requests +======================================= + +Before a PR can be landed into ``main``, 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 has been received from a **Committer**, and +* no "Request changes" reviews from any **Committer** are outstanding. + +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, and +* no "Request changes" reviews from any **Reviewer** are outstanding. + +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`_ 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 +rather an acknowledgement 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 +`Asking For Help`_ section above for the ways to do that. + +These are the steps that comprise the opening phase: + +1. The **Proposer** opens a 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 `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 `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 +`Inactive Pull Requests`_ for details about temporary closure. + +In this section, 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 ``main`` 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 to + 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 `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 :guilabel:`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 :guilabel:`"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 + straightforward :guilabel:`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 :guilabel:`"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 :guilabel:`Approve` vote or, for more complex PRs, + what the next steps towards an :guilabel:`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 `Opening phase`_ again, as it may +need some work done to get CI passing again. + +.. LINKS: +.. _channel on the Libera IRC: https://kiwiirc.com/nextclient/irc.libera.chat/cloud-init +.. _PR #237: https://github.com/canonical/cloud-init/pull/237 |