RackHD Code Review and Merge Process

Owned by ThomasS
Last updated: Jul 14, 2017 by Amy Mullins
4 min read

The intent of this page is to provide guidelines to “in-training” or new code committers as the RackHD development team expands the set of members with code committer privileges.

Code Readiness

At a minimum, code should be build-able, unit and functionally tested in a local developer environment or sandbox prior to being submitted for review.

Core Committer and Committer Definitions

  • A Core Committer has capabilities to merge code in all of the RackHD repositories.  They are responsible for:
    • Preserving RackHD design principles and architectural compliance to the design principles
    • Reviewing, Merging contributions from the external community   
    • Reviewing, Merging contributions from the internal community that have not leveraged the pair programming tool
    • Design conflict resolution
  •  A Committer has capabilities to merge code in all of the RackHD repositories and can merge code only when paired up on a feature, story or bug such that code is developed and reviewed by the pair.  If the feature, story, or bug is not developed leveraging the pair programming tool, two positive reviews with at least one of those reviews being one of the core committer and no negative votes (ie, the original process) must be followed.
    • Committers are also responsible for preserving RackHD design principles and architectural compliance to the design principles during code review.
  • A dev without committer status or pairing with a committer must follow the original process (two positive reviews with at least one of those reviews being one of the core committer and no negative votes) .

Reviewer and Committer Guidelines

Process Guidelines

    • A submitter cannot be the reviewer for the request he/she submitted.
    • A person who has paired with the submitter, worked with the submitter closely on the changes, or the team Anchor can be the reviewer and the committer (so long as commit privileges have been granted).
    • There is no limit on the number of reviewers since RackHD is an open source project.
    • Committers must pay attention to the pipeline to make sure no breaking changes to the pipeline were introduced as part of the PR quality gate and as part of the MasterCI nightly build..
    • In a pairing environment, the frequency of a review is expected to be near instantaneous.

Code Content Guidelines

    • Is the code sufficiently easy to follow?
    • Is the Code structure formatted correctly?
    • Are sufficient code comments provided to identify what the feature or fix is providing
    • Is the PR size reasonable?
      • If individual components of a feature/fix can be tested independently, then those component changes should be submitted individually once they are tested. The smaller the code change, the more efficient it is for the code reviewer. 
    • Code Duplication
      • Does the code use a lot of cut-paste code that should be refactored?
    • What public APIs are affected?
      • REST
      • Events
      • Graphs/Tasks/Jobs
    • For public API updates, are there any required documentation updates?.
    • Code should avoid divergence, for example if new parameter is added for a API, then the API's schema should be modified accordingly

Unit Test Guidelines

Unit tests are absolutely required before code can be claimed to be ready for code review.

For the developers:

    • Code is required to have unit tests associated with it.  It may not be feasible to generate unit tests for every line of code and may not be worth the time and effort.  At a minimum, unit tests should be provided for any code that has to make a decision or a computation.

For the Code Reviewers:

    • Significant drops in code coverage should be considered on exceptional cases only and should be communicated out to the RackHD Core Committer team if significant coverage changes are to be expected.  (ex  < 5%)
    • Unit tests should be submitted for code review in the same PR as the code changes. Reviewers should make a consensus call to decide whether enough unit tests are written/done for the changes submitted for code review before approving the changes.

Reference Travis CI for RackHD Unit Test Metrics: https://travis-ci.org/RackHD

Functional Test Guidelines

Required but can be staged such that technical debt is kept to a minimum.

Functional tests are to be developed in the FIT CI framework, ideally following a TDD approach where tests are created prior to the code being developed. 

Functional testing ensures core functionality has not been impacted by the requested code change and that the interfaces and dependencies are tested.

Functional Tests can run in a virtual hardware environment (such as the smoke tests in the PR quality gates or on physical hardware as in the nightly regression or extended testing)

Reference this link for additional Functional test criteria: https://hardware.corp.emc.com/display/gheonrack/RackHD+CI+Test+Conventions+-+Smoke%2C+Regression%2C+Extended+test+requirements

Test Content Guidelines

    • Are the smoke/regression labels applied correctly
    • Are these tests expected to run as PR quality gates (on a virtual hardware environment) or in a Regression environment such as MasterCI/ SprintRelease (on a physical hardware environment)  Jenkins PR tests?
    • Does it pass the PR gates
      • Code Coverage
      • Hound/Linters
      • Jenkins PR Tests
      • Travis Unit Tests
    • Committers must pay attention to the pipeline to make sure no breaking changes to the pipeline were introduced as part of the PR quality gate and as part of the MasterCI nightly build.
    • PR Format:
      • The Commit history should be simple with a reasonable number of commits, as we don't want a simple fix with tens of commit history
      • The commit message should briefly reflect the changes and reference a RAC issue (if applicable)