ReviewChecklist - Revision history

Smaffulli at 01:45, 4 December 2014

2014-12-04T01:45:43Z

Smaffulli: Moved page to new infra manual

2014-12-04T01:43:08Z

Moved page to new infra manual

Jpichon: /* Notes for Horizon Developers */

2014-11-04T17:06:47Z

‎Notes for Horizon Developers

Jpichon: Add link to Horizon review guidelines

2014-11-04T17:06:04Z

Add link to Horizon review guidelines

Smaffulli: added to Contribute

2014-09-26T00:27:30Z

added to Contribute

Mriedem: /* Common Review Checklist */

2014-06-19T18:42:29Z

‎Common Review Checklist

Fungi: /* Common Review Checklist */ updates for modernity

2014-04-01T12:58:09Z

‎Common Review Checklist: updates for modernity

Sahid: Add a note about using 'six'

2014-03-17T14:33:33Z

Add a note about using 'six'

Mriedem: /* Nova Review Checklist */

2014-02-14T20:53:48Z

‎Nova Review Checklist

Mriedem: /* Notes for Non-Core Developers */

2014-01-06T20:52:36Z

‎Notes for Non-Core Developers

← Older revision		Revision as of 01:45, 4 December 2014
Line 1:		Line 1:
−	#REDIRECT [[http://docs.openstack.org/infra/manual/developers.html#peer-review]]	+	#REDIRECT [http://docs.openstack.org/infra/manual/developers.html#peer-review OpenStack Infra Manual]
		+
		+	The content of this page is now hosted on [http://docs.openstack.org/infra/manual/developers.html#peer-review OpenStack Infra Manual]

← Older revision		Revision as of 01:43, 4 December 2014
Line 1:		Line 1:
−		+	#REDIRECT [[http://docs.openstack.org/infra/manual/developers.html#peer-review]]
−	This page includes a list of things that reviewers should keep in mind when reviewing patches to [[OpenStack]] projects. There are some items that are common across all projects and others that are specific to a project.
−
−	~~= Common Review Checklist =~~
−
−	# The code should comply with everything in that project's `HACKING.rst` file, if it has one. If the project reuses [https://git.openstack.org/cgit/openstack-dev/hacking/tree/HACKING.rst nova's hacking guideines], then it may have a "hacking" section in its `tox.ini` file in which case much of this is already checked automatically for you by the continuous integration system.
−	~~# The code should be 'pythonic' and look like the code around it, to make the code more uniform and easier to read.~~
−	~~# Commit Message and Change break-up:~~
−	## ~~Follow the advice of~~ [[~~GitCommitMessages]].~~
−	~~## Use the "DocImpact" tag on changes that affect documentation.~~
−	~~## Use the "SecurityImpact" tag on changes that should get the attention of the OpenStack Security Group (OSSG) for additional review.~~
−	~~## Use the "UpgradeImpact" tag on changes which require configuration changes to be mentioned in the release notes.~~
−	~~## If the patch fixes a bug, it should reference a bug report.~~
−	~~## If the patch implements a feature, it should reference a blueprint. The blueprint should be approved before the patch is merged.~~
−	~~# Test Case implementation (Mock vs. Mox):~~
−	~~## New test cases should be implemented using Mock. It is part of the Python standard library in Python 3 and as such is the preferred method for OpenStack.~~
−	~~## Exceptions can be made for tests added where Mox was already in use, or any other situation where using Mock would cause excessive difficulty for some reason.~~
−	~~## There is no need to convert existing Mox test cases to Mock, but if you are changing a Mox test case anyway, please consider converting it to Mock at the same time.~~
−	~~# About Python 3 compatibility:~~
−	~~## It is preferred for new code to use package six. When it is possible we should be use `six.text_type` or `six.text_binary` to cast or test value for unicode or str.~~
−	~~# The code should comply with the community [[LoggingStandards]].~~
−
−	~~= Oslo Syncing Checklist =~~
−
−	~~# When syncing from oslo:~~
−	~~## Ensure the proposed change has actually merged in oslo.~~
−	## It should provide some benefit, don't sync just to sync. Maybe it fixes a bug, or provides new functionality to be used. If providing new functionality it should have a dependent patch lined up to use that functionality.
−	~~## The commit message should list which oslo changes are being synced in.~~
−
−	~~= Nova Review Checklist =~~
−
−	~~#For REST API changes:~~
−	~~## The change should conform to [[APIChangeGuidelines]]~~
−	~~### Note that we can currently still make incompatible changes to the V3 API~~
−	~~## Produce a full API design in a blueprint in advance~~
−	~~## Follow the [[Nova/APIStyleGuide\|Nova API Style Guide]]~~
−	~~## Ensure there are new API samples produced using realistic data (they are used for documentation)~~
−	~~### If an API sample test template changes, ensure the doc samples are also updated.~~
−	~~### V2 api samples are added to ''nova/tests/integrated/test_api_samples.py''~~
−	~~### V3 api samples are added to an extension specific file in ''nova/tests/integrated/v3''~~
−	~~## If a bug fix is applied to the V2 or V3 API, check to see if it is appropriate to apply it to the V3 or V2 API as well~~
−	~~## New API methods should have an associated policy check for access control~~
−	~~### Check an entry has been added to ''nova/etc/nova/policy.json''~~
−	~~## V3 API specific checks~~
−	~~### Confirm the extension has been at least added to ''nova.api.v3_extensions'' in ''setup.cfg''~~
−	~~### A discoverable policy must be added for each extension or it won't be visible by default in the /v3/extensions listing. eg~~
−	~~#### "compute_extension:v3:os-access-ips:discoverable": "",~~
−	~~# When changing an RPC interface~~
−	## Make sure the version numbers are updated appropriately and that backwards compatibility is maintained. More info [https://github.com/openstack/oslo-incubator/blob/master/openstack/common/rpc/dispatcher.py#L18 here].
−	~~## If an existing method is being updated, make sure support for sending the old version of the method is still supported. Search for existing uses of "can_send_version()" for examples.~~
−	~~# When changing rootwrap filters~~
−	~~## Add Thierry Carrez to the review~~
−	~~## FIXME: add specific review checklist~~
−	~~# Criteria for new compute drivers~~
−	~~## Should support everything in the core OpenStack compute API~~
−	~~## Should have public CI doing functional testing (~~http://~~lists~~.openstack.org/~~pipermail~~/~~openstack-dev~~/~~2013-July/011280~~.html)
−	~~# Global code cleanup which should be a hacking rule~~
−	## For every patch that is going through the entire source tree to change something, which should be that way for all time, a hacking rule should be added within the same patch to make sure it doesn't come back. This can be done by adding a nova-specific rule to nova/hacking/checks.py, but if it should be a standard rule across all projects it should go into the [http://git.openstack.org/cgit/openstack-dev/hacking/ openstack-dev/hacking project] where it can be released in a library and each individual project can consume it.
−	~~# When to approve~~
−	~~## Every patch needs two +2s before being approved.~~
−	## ~~Its ok to hold off on an approval until a subject matter expert reviews it, such as for neutron, xen, or glance related changes.~~
−	~~## If a patch has already been approved but requires a trivial rebase to merge, you do not have to wait for a second +2, since the patch has already had two +2s.~~
−
−	~~= Notes for Horizon Developers =~~
−
−	* See [[Horizon/Reviews\|Horizon review guidelines]]
−
−	~~= Notes for Non~~-~~Core Developers =~~
−
−	* When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention (preferably in IRC) and letting them know there is a review with lots of positive reviews and needs final approval.
−
−	~~[[Category:contribute~~]]

@@ Line 64: / Line 64: @@
 = Notes for Horizon Developers =
-# See [[Horizon/Reviews|Horizon review guidelines]]
+* See [[Horizon/Reviews|Horizon review guidelines]]
 = Notes for Non-Core Developers =

@@ Line 61: / Line 61: @@
 ## Its ok to hold off on an approval until a subject matter expert reviews it, such as for neutron, xen, or glance related changes.
 ## If a patch has already been approved but requires a trivial rebase to merge, you do not have to wait for a second +2, since the patch has already had two +2s.
 = Notes for Non-Core Developers =

← Older revision		Revision as of 00:27, 26 September 2014
Line 65:		Line 65:

	* When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention (preferably in IRC) and letting them know there is a review with lots of positive reviews and needs final approval.		* When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention (preferably in IRC) and letting them know there is a review with lots of positive reviews and needs final approval.
		+
		+	[[Category:contribute]]

@@ Line 19: / Line 19: @@
 # About Python 3 compatibility:
 ## It is preferred for new code to use package six. When it is possible we should be use `six.text_type` or `six.text_binary` to cast or test value for unicode or str.
 = Oslo Syncing Checklist =

@@ Line 4: / Line 4: @@
 = Common Review Checklist =
-# The code should comply with everything in [https://github.com/openstack-dev/hacking/blob/master/doc/source/index.rst HACKING].
+# The code should comply with everything in that project's `HACKING.rst` file, if it has one. If the project reuses [https://git.openstack.org/cgit/openstack-dev/hacking/tree/HACKING.rst nova's hacking guideines], then it may have a "hacking" section in its `tox.ini` file in which case much of this is already checked automatically for you by the continuous integration system.
-# The code should be 'pythonic' and look like the code around it, to make the code more uniform and easier to read
+# The code should be 'pythonic' and look like the code around it, to make the code more uniform and easier to read.
-# When adding a new file:
+# Commit Message and Change break-up:
 ## Follow the advice of [[GitCommitMessages]].
 ## Use the "DocImpact" tag on changes that affect documentation.
 ## Use the "SecurityImpact" tag on changes that should get the attention of the OpenStack Security Group (OSSG) for additional review.
 ## If the patch fixes a bug, it should reference a bug report.
 ## If the patch implements a feature, it should reference a blueprint.  The blueprint should be approved before the patch is merged.
-# Test Case implementation (Mock vs. Mox)
+# Test Case implementation (Mock vs. Mox):
 ## New test cases should be implemented using Mock.  It is part of the Python standard library in Python 3 and as such is the preferred method for OpenStack.
 ## Exceptions can be made for tests added where Mox was already in use, or any other situation where using Mock would cause excessive difficulty for some reason.
 ## There is no need to convert existing Mox test cases to Mock, but if you are changing a Mox test case anyway, please consider converting it to Mock at the same time.
-# About Python 3 compatibility
+# About Python 3 compatibility:
 ## It is preferred for new code to use package six. When it is possible we should be use `six.text_type` or `six.text_binary` to cast or test value for unicode or str.

@@ Line 53: / Line 53: @@
 ## Should support everything in the core OpenStack compute API
 ## Should have public CI doing functional testing (http://lists.openstack.org/pipermail/openstack-dev/2013-July/011280.html)
 # When to approve
 ## Every patch needs two +2s before being approved.

← Older revision		Revision as of 20:52, 6 January 2014
Line 60:		Line 60:
	= Notes for Non-Core Developers =		= Notes for Non-Core Developers =

−	* When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention and letting them know there is a review with lots of positive reviews and needs final approval.	+	* When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention (preferably in IRC) and letting them know there is a review with lots of positive reviews and needs final approval.