Code Manager's Guide

Guidelines for Code Managers and Reviewers

Important Note

For all reviewers - only approve a pull request if are fine with the changes as they are. If changes are desired or required, please don't toggle the Approve option in the review.

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.)
• Is required documentation included in the PR?

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.lua 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.)

Testing requirements before merging a PR into develop

• Following two approvals, please ensure that the Jenkins automated test label, run_we2e_coverage_tests, has been added to the PR. Please note that the Jet tests will likely fail due to HPSS access. The Jet coverage WE2E tests will need to be run manually.
• If the Jenkins tests need to be aborted due to failures, please reach out to Kris Booker, Bruce Kropp, or Mark Potts to assist. Also, Jenkins tests can only be resubmitted through the Jenkins API (removing and adding the label won't resubmit the Jenkins tests). Please reach out to them to resubmit Jenkins tests for a previously failed and updated PR.
• The following tests will need to be manually run before merging:
  • Jet WE2E coverage tests
  • The MET_ensemble_verification_only_vx_time_lag WE2E test will need to be run manually on Hera GNU
  • The custom_ESGgrid_Great_Lakes_snow_8km WE2E test will need to be run manually on Jet

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)?
• Are there any unresolved conversations in the PR? Please reach out to reviewers who left comments to ensure that they are happy with the changes before merging.