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. Does the code actually address the linked blueprint or bug? If addressing a blueprint any deviation from the approved plan and UI mocks should be explained.
- 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.
- If it is unclear how to test the patch, instructions should be included in the commit message or as a comment on the review. If it's unclear how to test, kindly ask the author to add them and review when added.
- 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.
- Run the tests before you commit your changes. It's always good to test your changes and get instant feedback rather than waiting for CI to let radiate that information to you.
- Integration tests should not have increased failures.
- 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.
- 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