Collaboration: issues, pull requests and code review

How a change reaches main

On a team, nobody commits straight to main. Every change travels the same road, and each stop catches a different kind of mistake:

⤢ View

Issue, branch, pull request, checks, review, merge. Rules on main make the checks and the review compulsory.© Diagram: DevOps Academy

Close

Branching strategies

Every team agrees, out loud or not, on two things: how long a branch may live, and what main means. That agreement is a branching strategy. Git doesn't care which one you pick; your release process and your merge conflicts do. There are three you'll meet at work:

⤢ View

Trunk-based keeps branches tiny. GitHub flow adds a pull request per change. GitFlow adds develop and release branches, and main only holds releases.© Animation: DevOps Academy

Close

Trunk-based development attacks the real cause of merge pain: branches that drift apart for weeks. Everyone merges small changes into main (the trunk) at least daily. A feature that isn't finished still merges, switched off behind a feature flag, so main stays releasable. It needs fast, trustworthy CI, because there's no long branch to catch mistakes on.

GitHub flow is trunk-based with a pull request on every change: branch from main, commit, open a PR, let CI and a reviewer pass it, merge, deploy. Branches live days, not weeks. It's what this course uses.

GitFlow was designed for software shipped as numbered releases. Features merge into develop; when it's time to ship, a release/1.1 branch is cut, only fixes go into it, and it merges into main (tagged v1.1) and back into develop. Urgent fixes branch from main as hotfix/*. You get a stabilisation period, at the cost of more branches, more merges and slower feedback. For a web service you deploy continuously, it's usually more process than you need; its own author later added a note saying the same.

Pick by how you ship. Deploying continuously points to GitHub flow or trunk-based. Shipping versions that customers install, with several versions supported at once, is where release branches earn their keep.

Issues and labels

An issue is a unit of work: a bug, a feature, a question. A good one can be picked up by someone who wasn't in the conversation:

Title: Checkout crashes when the cart is empty

Steps to reproduce
1. Open /cart with no items
2. Click "Checkout"

Expected: a "Your cart is empty" message
Actual:   500 error, ZeroDivisionError in checkout.py line 88

Version: 1.4.0, Chrome 130

Labels make a long list searchable: bug, enhancement, good first issue, priority: high. Assignees say who is on it; milestones group issues for a release. Templates in .github/ISSUE_TEMPLATE/ give every new issue the right questions, so you get steps to reproduce without asking for them.

$ gh issue create --title "Checkout crashes when the cart is empty" --label bug
$ gh issue list --label bug --assignee @me
$ gh issue view 42

The pull request flow

A pull request (PR) says "here is a branch; please review it and merge it into main". It holds the diff, the discussion, the review and the test results in one place.

$ git switch -c fix/42-empty-cart
$ git commit -am "Show a message instead of crashing on an empty cart"   # after editing and testing
$ git push -u origin fix/42-empty-cart
$ gh pr create --fill --draft      # title and body from your commits; draft = not ready yet
$ gh pr ready                      # when it is ready for review
$ gh pr status                     # your pull requests and their checks
$ gh pr merge --squash --delete-branch

A description reviewers can work with:

## What
Show "Your cart is empty" instead of a 500 when checking out with no items.

## Why
Closes #42. Checkout divided by the item count to show an average price.

## How to test
1. Empty the cart, click Checkout: you see the message.
2. Add an item: checkout works as before.

## Screenshots
(before / after)

Closes #42 (or Fixes, Resolves) links the issue and closes it when the PR merges into the default branch. A .github/pull_request_template.md pre-fills these headings in every new PR.

Keep PRs small. Reviewers do their best work on about 200–400 changed lines. A 2,000-line PR gets a skim and "LGTM". Split big work into several PRs that each leave main working.

When review asks for changes, push more commits to the same branch: the PR updates itself. If main moved on, update your branch (rebase it, or the Update branch button) so checks run against current code.

Pull requests from a fork

When you can't push to the repository (open source, another team), the PR comes from your fork:

$ gh repo fork owner/project --clone && cd project    # origin = your fork, upstream = original
$ git switch -c fix/typo-in-readme
$ git commit -am "Fix the install command in the README"
$ git push -u origin fix/typo-in-readme
$ gh pr create --repo owner/project --fill            # PR from your fork to the original

Before starting, read the project's CONTRIBUTING.md and check an issue exists or open one: a maintainer may already have plans. Keep your fork current with git fetch upstream and git rebase upstream/main. Leave Allow edits by maintainers ticked so they can fix small things on your branch.

Code review that helps

As the reviewer, you're checking that the change is correct, safe and maintainable, not that it's written the way you would have:

Write comments people can act on, and say how important they are:

blocking: This runs one query per cart item. Can we load them in one query with an IN clause?
suggestion: `items_total` might read better than `t`.
nit: trailing space.
question: Why 30 seconds here? A constant with a name would explain it.

Use suggested changes (the ± button on a line) for small fixes the author can apply in one click. Finish with Approve, Request changes or Comment: gh pr review --approve.

As the author, review your own diff first, answer every comment with a fix or a reason, and don't take it personally: the review is about the code.

Protecting main

Turn the good habits into rules. In Settings → Rules → Rulesets (or the older Branches → Branch protection), create a rule for main:

CODEOWNERS

A team of twenty can't have everyone review everything, and "someone approved it" isn't the same as "the people who run the database approved the migration". CODEOWNERS maps paths to the people or teams responsible for them. GitHub reads it from .github/CODEOWNERS (or the repository root, or docs/) on the base branch of the pull request, and requests reviews from the matching owners automatically:

# .github/CODEOWNERS
# Order matters: for each file, the LAST matching line wins.

# Fallback owner for everything
*                     @rajeev

# Terraform and Kubernetes manifests; CI workflows can reach deploy secrets
/infra/               @rajeev/platform-team
/.github/workflows/   @rajeev/platform-team

*.sql                 @rajeev/db-team
/services/payments/   @rajeev/payments-team @rajeev/security-team

# No owner: anyone with write access can approve docs
/docs/

A pull request that changes services/payments/api.py and infra/rds.tf asks for both the payments team and the platform team. Because the last match wins, put the broad * line first and the specific paths after it. A line with a path and no owner removes ownership, which is how you exempt docs/. Owners must have write access to the repository, and teams must be visible.

On its own, CODEOWNERS only requests reviews. It becomes a gate when you pair it with Require review from Code Owners in the ruleset: then the pull request can't merge until an owner of every changed path has approved. If the file has a mistake, such as a team that doesn't exist, GitHub shows the error when you open CODEOWNERS on the website, so check it after every edit.

Branch names from the Branches lesson (feature/, fix/, hotfix/) matter here too: rulesets can target patterns such as release/*, and CI can deploy by branch name.

Merge queue

Protecting main still leaves one gap. Two pull requests are each green, each tested against main as it was when their checks ran. Merge the first, and the second is now going into a main its tests never saw. One renamed a function; the other added a new call to the old name. Both passed, and main is broken.

The usual fix, Require branches to be up to date before merging, closes the gap but creates a treadmill on a busy repository: every merge makes every other open pull request out of date, so each one has to update and wait for CI again, and whoever merges first wins.

A merge queue does that waiting for you. Instead of merging, you add the pull request to the queue (gh pr merge --auto, or the Merge when ready button). GitHub builds a temporary branch containing main plus the pull requests ahead of yours plus yours, runs the required checks on that combination, and merges only if they pass. If one fails, it's removed from the queue and the rest carry on without it.

# .github/workflows/ci.yml: the checks must also run for the queue's temporary branches
on:
  pull_request:
  merge_group:

Turn it on with Require merge queue in the ruleset for main. It's worth it once several pull requests merge into the same branch each day. On a team of three, the up-to-date rule is enough.

$ gh issue create --title "README doesn't say how to run the app" --body "Add the run command." --label documentation
https://github.com/rajeev/shop/issues/1
$ git switch main && git pull
$ git switch -c docs/1-run-command
$ echo 'Run it with: `python app.py`' >> README.md
$ git commit -am "Document how to run the app"
$ git push -u origin docs/1-run-command
$ gh pr create --title "Document how to run the app" --body "Closes #1"
$ gh pr view --web                       # read the diff as a reviewer would
$ gh pr merge --squash --delete-branch
$ gh issue view 1 --json state --jq .state
CLOSED
$ git switch main && git pull && git log --oneline -1