|
...
|
...
|
@@ -320,3 +320,48 @@ Variables and Functions
|
|
320
|
320
|
- function names should_have_underscores, NotCamelCase.
|
|
321
|
321
|
- functions should be declared as per the regex ^function foo {$
|
|
322
|
322
|
with code starting on the next line
|
|
|
323
|
+
|
|
|
324
|
+
|
|
|
325
|
+Review Criteria
|
|
|
326
|
+===============
|
|
|
327
|
+
|
|
|
328
|
+There are some broad criteria that will be followed when reviewing
|
|
|
329
|
+your change
|
|
|
330
|
+
|
|
|
331
|
+* **Is it passing tests** -- your change will not be reviewed
|
|
|
332
|
+ throughly unless the official CI has run successfully against it.
|
|
|
333
|
+
|
|
|
334
|
+* **Does this belong in DevStack** -- DevStack reviewers have a
|
|
|
335
|
+ default position of "no" but are ready to be convinced by your
|
|
|
336
|
+ change.
|
|
|
337
|
+
|
|
|
338
|
+ For very large changes, you should consider :doc:`the plugins system
|
|
|
339
|
+ <plugins>` to see if your code is better abstracted from the main
|
|
|
340
|
+ repository.
|
|
|
341
|
+
|
|
|
342
|
+ For smaller changes, you should always consider if the change can be
|
|
|
343
|
+ encapsulated by per-user settings in ``local.conf``. A common example
|
|
|
344
|
+ is adding a simple config-option to an ``ini`` file. Specific flags
|
|
|
345
|
+ are not usually required for this, although adding documentation
|
|
|
346
|
+ about how to achieve a larger goal (which might include turning on
|
|
|
347
|
+ various settings, etc) is always welcome.
|
|
|
348
|
+
|
|
|
349
|
+* **Work-arounds** -- often things get broken and DevStack can be in a
|
|
|
350
|
+ position to fix them. Work-arounds are fine, but should be
|
|
|
351
|
+ presented in the context of fixing the root-cause of the problem.
|
|
|
352
|
+ This means it is well-commented in the code and the change-log and
|
|
|
353
|
+ mostly likely includes links to changes or bugs that fix the
|
|
|
354
|
+ underlying problem.
|
|
|
355
|
+
|
|
|
356
|
+* **Should this be upstream** -- DevStack generally does not override
|
|
|
357
|
+ default choices provided by projects and attempts to not
|
|
|
358
|
+ unexpectedly modify behaviour.
|
|
|
359
|
+
|
|
|
360
|
+* **Context in commit messages** -- DevStack touches many different
|
|
|
361
|
+ areas and reviewers need context around changes to make good
|
|
|
362
|
+ decisions. We also always want it to be clear to someone -- perhaps
|
|
|
363
|
+ even years from now -- why we were motivated to make a change at the
|
|
|
364
|
+ time.
|
|
|
365
|
+
|
|
|
366
|
+* **Reviewers** -- please see ``MAINTAINERS.rst`` for a list of people
|
|
|
367
|
+ that should be added to reviews of various sub-systems.
|