1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
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
|
