porter icon indicating copy to clipboard operation
porter copied to clipboard

Clarify requirements around reviews

Open carolynvs opened this issue 2 years ago • 0 comments

Let's define in our REVIEWING.md file the requirements around when a code review is required, who can review code, when it's okay to merge without a qualifying review, and when build checks can be skipped (e.g. can you skip the DCO check ever? What about the main build? Docs build?) Are there different rules for different repos?

Briefly, here is how we operate today. I want to document how we work, but not change how we work.

  • We are a small project and do not have enough maintainers/reviewers to have everything reviewed by 2 maintainers. This is a process that we have come to in order to not be blocked for long stretches of time and keep the project running.
  • Only maintainers can merge code.
  • Pull requests from maintainers optionally are reviewed by another maintainer. See below for more detail.
  • Always use a pull request, and do not push directly to main or a release branch.
  • Changes to the functionality of Porter should always be reviewed.
  • At the maintainers discretion they can count a review from someone who is not a maintainer and use that to merge. We are working on a formal process for recognizing people as reviewers who are not maintainers.
  • Changes to Porter's infrastructure may be merged without review. We'd prefer reviews but fixing builds, or making other changes to keep the project running smoothly can't be held up due to lack of reviewers.
  • Do not skip the DCO check. Coach a committer on fixing their commit. When the committer has signed some but not all of their commits (this often happens when they accept a suggested change through the GH UI) you can squash and then all the commits are signed.
  • The doc build must be passing if the PR changed the website.
  • The porter-integration build must be passing to merge. If a test is flaky, create an issue for it and retrigger the build until green.
  • The porter build must pass.
  • Code coverage is something we strive for but at the discretion of the maintainer, we can ignore the failed codecov build.
  • CVEs identified during a pull request should be fixed in a separate PR unless its relevant to that PR. (i.e. open a new PR and bump your deps to get a green build, then merge those changes and use them in the original PR).

carolynvs avatar Mar 29 '22 15:03 carolynvs