Skip to content

Code Manager's Guide

Gillian Petro edited this page Aug 17, 2022 · 18 revisions

Guidelines for Code Managers and Reviewers

General

  • Is the developer's branch updated to HEAD of develop?
  • Is the branch appropriately named using a bugfix/, feature/, or text/ prefix? (If not, remind user.)
  • Is there an open issue that this PR addresses? (Code manager discretion: Ask user to create issue or remind user to do so next time.)
  • Have appropriate labels been added to the PR? (This is most important if the contributor does not have permissions to add labels.)
  • If this is a regional_workflow PR, has a corresponding PR to ufs-srweather-app been opened to update the regional_workflow hash and any required documentation?

Code

  • Does the code contribution follow the Code and Configuration Standards laid out in the Contributor's Guide wiki? For example:
    • Externals point to appropriate hash of authoritative repositories.
    • Platform-specific settings are handled only through configuration and modulefiles, not in code or scripts.
    • SRW: Components build using the common modules located in the modulefiles/srw_common file.
    • Regional Workflow: All bash scripts must explicitly be #!/bin/bash scripts (and not login enabled).
  • Does the contribution follow NCO Guidelines for the scripts, jobs, and ush directories?
  • Does the code break supported capabilities on any supported platforms?

Testing

  • Has the PR been sufficiently tested?
    • Was at least one end-to-end test run on at least one supported platform?
    • Has all new functionality been tested explicitly?
    • Which platform(s) did the user test on?/Which platforms still require testing?
    • Did the user run the fundamental test suite? (Code manager discretion: in minor PRs, running a subset of tests may be acceptable.)
    • Which (if any) comprehensive tests were run? (Code manager discretion whether to require some/all comprehensive tests.)

Documentation

  • Did the user update the .rst documentation files with information related to their PR?
  • If not, and documentation is required, did the user open an issue to add documentation?

Merging

  • Is at least one of the code reviewers a code manager? (If not, request that a code manager review the PR before merging.)
  • If code has changed since the PR was opened, ensure that appropriate tests were re-run.
  • Did the code owner conducting the merge check with the PR author to ensure that it is ready for merging (e.g., no last-minute minor fixes coming in)?