Skip to content
This repository has been archived by the owner on Aug 30, 2024. It is now read-only.

Code Review Criteria

Jump to bottom
Kamal Gill edited this page Sep 15, 2016 · 7 revisions

Documented here is the criteria for a pull request (PR) to pass a code review. This will help developers perform pre-flight checks before opening a PR and will provide reviewers a consistent set of criteria when examining code in a PR.

First things first

  • Features and bug fixes must be tested across Eucalyptus and AWS (unless the feature is Euca-only).
  • Features and bug fixes should be tested in the major browsers we support (IE 11, Edge, and latest Chrome, Firefox and Safari), and the browsers/devices tested on should be listed in the PR's description or the Jira ticket.
  • Features and bug fixes requiring UX review must pass UX review prior to submitting to QA. The UX review may be requested before or after code review, depending on the scope of the ticket and the availability of the UX reviewer.
  • Features in a PR should be only relevant to a single ticket. In a few cases, multiple related tickets may be combined in a single PR where appropriate (e.g. if a single fix spans multiple tickets), but unrelated tickets must not be combined.

Criteria for all code

  • Code should use an indentation level of 4 spaces (not tabs!).

Criteria for Python

  • Python modules must be PEP8-compliant (with line breaks at 120 char instead of 80).
  • Strings that are displayed in the interface must be marked for i18n.
  • Unused variables and imports must be removed.
  • Stray comments and print statements must be removed.
  • Unit tests must be provided for new features and should be provided for bug fixes.
  • Python-based Selenium tests should be provided.
  • Bonus points for adhering to the style guide at https://github.com/amontalenti/elements-of-python-style for conventions not covered by PEP 8

Criteria for HTML

  • HTML must be XHTML-compliant to avoid breaking i18n and potentially causing unexpected rendering problems in the browser.
  • Each id attribute for a given HTML page must be unique, meaning a page must not contain two or more elements with the same id attribute.
  • HTML tags that wrap words must be marked for translation via the i18n:translate attribute, and the tag that contains the i18n:attribute must not have child tags.
  • HTML code indentation level should be 4 spaces when possible. If a file has many levels of nesting, 2 spaces may be used for indentation, as long as the entire file is consistent.
  • Inline JS (within script tags and in HTML attributes) must be avoided.
  • References to JS files in script tags must use the minified version of the file when available.
  • HTML attributes must use double quotes, not single quotes

Criteria for JS

Criteria for AngularJS

  • AngularJS code should follow the style guide at https://github.com/johnpapa/angular-styleguide (eases transition to Angular 2)
  • Only those methods and members that are explicitly bound to HTML should be stored on $scope. The rest may be safely defined and used within the app or directive controller.

Criteria for SCSS/CSS

  • Selectors should use classes rather than ids when possible for reuse.
  • SCSS files should avoid more than two levels of nesting.
  • Inline styles (within style tags and in HTML attributes) must be avoided.
  • Existing styles should be reused or extended rather than creating new styles.

Security Criteria

  • All user-generated input that is displayed on the page must be invulnerable to Angular expression injections and should be escaped by one of three ways: Passing the input to the template as a string (for read-only display), escaping server-side via BaseView.escape_braces(), or escaping client-side via an ng-non-bindable attribute.

GitHub PR Formatting and Jira Workflow

  • The GitHub pull request must contain a descriptive title prefixed with the Jira ticket(s), and the description must contain a link to the Jira ticket. See https://github.com/eucalyptus/eucaconsole/pull/1337 for an example.
  • The Jira ticket must include a link to the pull request, and the ticket should be assigned to the reviewer via the "Submit Patch" operation.
  • Unless the PR changes are fairly trivial, the pull request and Jira ticket should be passed to a second reviewer. Code reviews of non-trivial pull requests should have at least two stamps of approval to merge. As the term "non-trivial" is a bit vague here, when in doubt the reviewer should ask another developer if they want to also code review the PR.

Essential Resources

Clone this wiki locally