Skip to content

Contributing to the Code

Jump to bottom
ridz1208 edited this page Oct 23, 2017 · 9 revisions

Making a Pull Request

Preamble

As part of the open source community on GitHub, LORIS welcomes any contribution to the code issued by collaborators or any other member of the open source community. The following document is meant to aid in understanding the process of issuing pull requests to the LORIS codebase. Following the guidelines below helps streamline the review process and allows for safer and faster release cycles.

Definitions

GitHub offers several systems for organising Pull Requests and Issues. LORIS uses these systems as defined below.

  • Reviewers: Members selected for code review of the Pull Request in question.
  • Assignees: Members selected for manual testing of the Pull Request in question.
  • Milestones: Target release version for the PR.
  • Labels: Set of descriptors facilitating processing of a PR.
  • Projects: Tool grouping Pull Requests by there stage in the code review project. See Code Review rules for more details.
  • Base Branches: Descriptors of the nature of the pull request. Bugfix, minor or major.

Procedure

In order for a Pull Request to be processed as quickly as possible, it is crucial for each party involved to fulfill their responsibilities as follows.

The tasks below lie with the Developer issuing the Pull Request:

  1. Choosing the correct branch to issue the Pull Request. See Choosing Branch and [branch] label section below
  2. Provide a clear description for the Pull Request. Following the suggested template is generally the best option.
  3. Adding any necessary related PRs, tickets or context documents.
  4. Selecting the [branch] label that matches the branch and thus reflects the type of code changes.
  5. Selecting appropriate labels. Labels are explained in detail below.
  6. When possible selecting a reviewer. The best reviewers are the module experts (often suggested automatically on GitHub).

The tasks below lie with the Senior Developers and Repository Managers:

  1. Confirm choice of Branch, Reviewers and Labels.
  2. Make suggestions for testers through comments. Different sections of codes can have different independent testers.
  3. Assign the Pull Request to a Milestone to reflect which LORIS release it will be in.

Choosing Branch and [branch] label

Branch and label selection will depend on the type of changes in the code:

  • Bug Fixes

    • Branch: bugfix
    • Label: [branch] bugfix
    • Content: Generally these changes do not require SQL scripts and are concise with the sole objective to correct on single problem in the code.

  • Minor Changes and Small Features

    • Branch: minor
    • Label: [branch] minor
    • Content: Features affecting self-contained components such as modules. Additions to Libraries, API or modules that do not change and function signatures.

  • Major Changes, Non Backwards-Compatible Changes and Large Features

    • Branch: major
    • Label: [branch] major
    • Content: Any change modifying a function signature in a library class. Features require extensive LORIS-wide testing. New complex systems and features spanning across multiple modules and libraries. Deprecated functions clean-up.

Special cases

  • When a Pull Request contains multiple types of changes, it is suggested to divid the PR into multiple smaller ones when possible and issuing them separately; smaller pull requests are easier to review and tend to contain less of a risk of damage to the remainder of the code.

  • In the event that a Pull Request can not be subdivided into smaller ones, it should be categorized in respect to the most impacting change it contains (i.e. a PR containing both bug fixes and minor features would be considered as a minor feature request and would then be issued to the minor branch).

Considerations

  • Label-branch Hack:

    • GitHub page: Pull Requests
    • Problem: GitHub does not allow searching or filtering by base branch selected for a PR.
    • Temporary Solution: 3 additional labels created [branch] bugfix, [branch] minor and [branch] major use solely to reflect which branch the Pull Request is going to. Exactly one of these labels should be added on each PR.
    • Conditions for removal: GitHub adding filtering capabilities on repository branches.

  • Milestone-Project Hack:

    • GitHub page: Project View
    • Problem: GitHub does not allow searching or filtering cards by Milestone.
    • Temporary Solution: Create one Code Review project per anticipated release.
    • Conditions for removal: GitHub adding filtering capabilities for cards by Milestone.

Understanding Labels (Developer and Repository manager)

The following GitHub Labels are designed to be mutually-exclusive. The choice of Label should directly reflect the base branch the Pull request has been issued to. This label should be modified if base branch is changed under any circumstance.

[branch] bugfix

[branch] minor

[branch] major

The following GitHub Labels are designed to be non mutually-exclusive. Proper usage of these options facilitates triage, categorization and processing of a PR. Every flag added should have an associated section in the main description of the Pull Request justifying the requirement for the aforementioned flag; failure to so can delay the processing of the PR.

Add to Release Notes

  • Description: PR change should be highlighted in Release notes, such as a key security fix, a new dependency, a new practice enforced by the codebase, or other significant change. Not always but sometimes overlaps Feature or Caveat... tag. (Examples)

API

  • Description: PR introduces Modifications to the API code.

Blocked

  • Description 1: PR awaiting the merge, resolution or rejection of an other Pull Request.
  • Description 2: PR flagged for potential rejection.

Bug Fix

  • Description: PR contains bug fixes (not mutually exclusive with the Feature label).

Caveat for Existing Projects

  • Description: PR contains changes that might impact the code or accepted practices of current active projects. Any non backwards-compatible change should have this flag. Any change to library function signatures should have this flag.

Cleanup

  • Description: PR consists of at least one clean-up operation (could be backwards-compatible or not).

Critical to release

  • Description: PR is key for the release to which it has been assigned.

Discussion Required

  • Description: PR awaiting the resolution of a discussion between all involved parties. Discussion should occur on the conversation page of the PR. If discussion is done offline, a short summary is expected to appear on the conversation page.

Document for Release

  • Description: PR adds or changes a feature such that the wiki (or other documentation) must be updated. e.g. new module needs a wiki page, change to install process, imaging pipeline execution changes.

Documentation

  • Description: PR contains modifications to the code documentation including test plans and wikis.

Feature

  • Description: PR implements at least one new feature (not mutually exclusive with the Bug Fix label)

"Help! I don't understand Travis!"

  • Description: TRAVIS disagrees with PR for mysterious reasons.

Meta

  • Description: PR does something that organizes, upgrades, or manages the functionality of the codebase. (Examples)

Needs Rebase

  • Description 1: PR issued to the correct branch but contains conflicts with the current state of the target branch.
  • Description 2: PR issued to the wrong branch.

Needs Work

  • Description: PR awaiting additional changes by the author or contains issues that the author needs to fix.

Passed Manual Tests

  • Description: PR has undergone proper testing by at least one peer.

SQL

  • Description: PR contains SQL modifications such as schema changes and new SQL scripts.

Testing

  • Description: PR contains test plan or automated test code (or config files for Travis)

UI

  • Description: PR alters or improves the current User Interface.

Home | About | Community | Setup | Technical

Clone this wiki locally