Difference between revisions of "Horizon/Reviews"
|Line 3:||Line 3:|
== Review Criteria ==
== Review Criteria ==
Patches be reviewed from four perspectives.
=== Code ===
=== Code ===
Revision as of 18:44, 9 July 2015
Horizon Code Reviews
Patches be reviewed from four perspectives: code, tests, documentation and deployment.
- 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 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.
- 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.
- 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?
Topics related to code reviews related to Horizon (including django-openstack-auth, tuskar-ui).
Please add good references useful for reviews.
- OpenStack guidelines for code reviewers
- OpenStack Style Guidelines
- Git Commit Messages
- About commits into a stable branch (backports)
- Horizon + django-openstack-auth Review Dashboard
- Gerrit Dashboard Creator is useful.
- Dashboard #1
- Generated from Created by https://github.com/stackforge/gerrit-dash-creator/tree/master/dashboards
- Another Review Dashboard (which covers Horizon, django-openstack-auth and tuskar-ui)
- Generated from the dashboard definition below:
[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