Jump to: navigation, search

Difference between revisions of "Horizon/Reviews"

(Code)
(Documentation)
Line 23: Line 23:
  
 
=== Documentation ===
 
=== Documentation ===
* Is the change documented? Patches that change/add settings should be documented. Patches that change how underlying widgets work should be documented. New features like widgets that are intended for reuse should be documented.
+
* Is the change documented? Patches that change/add settings should be documented in [http://git.openstack.org/cgit/openstack/horizon/tree/doc/source/topics/settings.rst horizon docs]. Patches that change how underlying widgets work should be documented in horizon/doc as well as code. New features like widgets that are intended for reuse should be documented.
  
 
=== Deployment ===
 
=== Deployment ===

Revision as of 19:20, 9 July 2015

Horizon Code Reviews

Review Criteria

Patches be reviewed from four perspectives: code, tests, documentation and deployment.

Code

  • Is the code correct? Is it well formed and does it match the style guidelines linked below?
  • Does it work?
  • Does it address one issue, or more than one? Only one issue should be addressed per patch. This becomes slightly muddy when doing things like CSS cleanup, or implementing a larger feature. What this is intended to restrict is two unrelated changes being merged in one patch. This makes change tracking very difficult and should something like a revert or cherry-pick to the stable branch be necessary, we'll be reverting multiple changes rather than just the problematic one.
  • The code being reviewed is related to a single change. If you find issues with code surrounding the change, but that is unrelated, that is not a valid review comment. Please file a bug in launchpad to have the issue addressed separately.
  • Is the change beneficial? Excessive code thrash to cater to personal preferences is not beneficial overall. Changes like this are more disruptive than anything.
  • Is this change reimplementing functionality that is already available? We don't need/want 3 implementation of the same functionality.
  • If the patch includes new/changed calls to a python-*client, is that feature supported by the minimum supported version of that client in openstack/requirements. If not, is there feature/extension checking included?
  • Does the commit message reference the bug-id or blueprint being addressed? All changes should be tracked in launchpad as well.
  • If you are unsure about the subject matter of the change, ask the functional expert in Horizon to review, or ask other cores. Feel free to ask questions in the review.
  • All user visible strings must be translatable.

Tests

  • If it is unclear how to test the patch, instructions should be included in the commit message or as a comment on the review.
  • Does the patch have tests? All patches should be validated with unit tests. This protects against regressions in the future. Including tests benefits the author and the project. Code re-organization, cleanup or movement may be exempt. But all tests should still pass.
  • Integration tests should not have increased failures.

Documentation

  • Is the change documented? Patches that change/add settings should be documented in horizon docs. Patches that change how underlying widgets work should be documented in horizon/doc as well as code. New features like widgets that are intended for reuse should be documented.

Deployment

  • Consider whether this change will impact already deployed versions of Horizon and envision the upgrade path a deployer will need to exercise to absorb these changes. If a migration will be required, is this documented?
  • Large parts of Horizon are overridden by deployers. Is this patch changing the names of CSS classes, python classes, template paths, HTML elements, JavaScript methods, etc for purely cosmetic reasons. If so, those changes should be avoided. If there is a strong reason to change, that should be included in the commit message.

References

Topics related to code reviews related to Horizon (including django-openstack-auth, tuskar-ui).

Please add good references useful for reviews.

Review Links

 [dashboard]
 title = Horizon Review Inbox
 description = Review Inbox
 foreach = (project:openstack/horizon OR project:openstack/django_openstack_auth OR project:openstack/tuskar-ui) status:open NOT owner:self NOT label:Workflow<=-1 label:Verified>=1,jenkins NOT label:Code-Review>=0,self
 
 [section "Needs Feedback (Changes older than 5 days that have not been reviewed by anyone)"]
 query = NOT label:Code-Review<=2 age:5d
 
 [section "Your are a reviewer, but haven't voted in the current revision"]
 query = NOT label:Code-Review<=2,self reviewer:self
 
 [section "Needs final +2"]
 query = label:Code-Review>=2 limit:50
 
 [section "Passed Jenkins, No Negative Feedback"]
 query = NOT label:Code-Review>=2 NOT label:Code-Review<=-1 limit:50
 
 [section "Wayward Changes (Changes with no code review in the last 2days)"]
 query = NOT label:Code-Review<=2 age:2d