Skip to content

Latest commit

 

History

History
138 lines (107 loc) · 5.42 KB

REVIEWING.md

File metadata and controls

138 lines (107 loc) · 5.42 KB
Raw

Reviewing Guide

Values

Thank you for reviewing pull requests for us! 💖

Here are the values and etiquette that we follow to ensure a welcoming, inclusive project that doesn't burn out our authors or our reviewers. 😅

  • We ask that authors respect reviewers time. Check out the Contributing Guide and know that you can ask the author to do their part to make your part managable.
  • We ask that reviewers respect authors time. Please do your best to review a pull request in a reasonable amount of time once you have assigned it to yourself.
  • The definition of "reasonable amount of time" is 3 business days. The ask is that after each trigger: "Review Requested", "Changes Incorporated", etc that the other person attempt to do their part within 3 business days. If they can't, please leave a comment and let the other person know that it will take longer. If life comes up, let others know that you need to unassign yourself and someone else will complete the review.

How to review a pull request

  1. Do not start reviewing a pull request if it is in WIP or is a draft pull request. Wait until they have marked it ready for review.

  2. Assign yourself to the pull request. This gives the author feedback that someone is going to do the review, while giving you time to do the review.

  3. Do a quick check for areas that need to be addressed before the pull request can be reviewed.

    For example, it is missing an agreed upon solution, requires an explanation from the author, has a very large set of changes that are not easy to review, etc., ask the author to correct that up-front.

  4. When you provide feedback, make it clear if the change must be made in order for the pull request to be approved, or if it is just a suggestion. Mark suggestions with nit, for example nit: I prefer that the bikeshed be blue.

  5. When the pull request is ready to merge, squash the commits they require tidying unless the author asked to do that themselves.

See The life of a pull request for what we expect a pull request to feel like for everyone involved.

Merge Requirements

  • Unit Tests
  • Documentation Updated
  • Passing CI

When a pull request impacts code, i.e. it's not a documentation-only change, the reviewer should run the manual integration tests after reviewing the code. The tests are triggered with a comment:

/azp run porter-integration

Admins are allowed, at their discretion, to merge administrative pull requests without review and before the full CI suite has passed. This is sometimes used for typo fixes, updates to markdown files, etc. This is a judgement call based on the type of change, risk, and availability of other reviewers.

Giving feedback

  • Be kind. Here is good article with example code reviews and how to improve your feedback. Giving feedback of this caliber is an requirement of maintainers and those who cannot do so will have the maintainer role revoked.
  • Request changes for bugs and program correctness.
  • Request changes to be consistent with existing precedent in the codebase.
  • Request tests and documentation in the same pull request.
  • Prefer to optimize for performance when necessary but not up-front without a reason.
  • Prefer follow-on PRs.
  • Do not ask the author to write in your style.

Cut a Release

🧀💨

Our CI system watches for tags, and when a tag is pushed, it executes the publish target in the Makefile. When you are asked to cut a new release, here is the process:

  1. Figure out the correct version number, we follow semver:

    • Bump the major segment if there are any breaking changes.
    • Bump the minor segment if there are new features only.
    • Bump the patch segment if there are bug fixes only.
    • Bump the build segment (version-prerelease.BUILD) if you only fixed something in the build, but the final binaries are the same.

  2. First, ensure that the master CI build has already passed for the commit that you want to tag, and has published the canary binaries.

    Then create the tag and push it:

    git checkout master
git pull
git tag VERSION -a -m ""
git push --tags

  3. Generate some release notes and put them into the release on GitHub. The following command gives you a list of all the merged pull requests:

    git log --oneline OLDVERSION..NEWVERSION

    You need to go through that and make a bulleted list of features and fixes with the PR titles and links to the PR. If you come up with an easier way of doing this, please submit a PR to update these instructions. 😅

    # Features
* PR TITLE (#PR NUMBER)

# Fixes
* PR TITLE (#PR NUMBER)

# Install or Upgrade
Run (or re-run) the installation from https://porter.sh/install to get the 
latest version of porter.

  4. Name the release after the version.