• Thomas Burleson, Solutions Architect
• Copyright 2022 - All rights reserved

Knowledge Pyramid

The illustration below highlights the layered training process used to scale teams to Enterprise Git Best Practices:

Git Guidelines: Pull Requests

Developers are often confused by the concept of Git Pull Requests. While the concept is simple, team often struggle with Pull Requests (PRs). There are, however, best practices that will ensure your PR process is pain-free and clean.

• Use recommended GitFlow Feature Branches.
• Use Pull-Request Best Practices (this document)
• Use Code Review Best Practices

Why Use Pull Requests?

So when do developers need to use PRs?

Pull requests are critical when the author (developer making the changes) wants to:

1. Merge code from the current feature branch to another branch
2. Share ideas & approach with specific developers
3. Request code reviews from specific developers
4. Demonstrate quality and standards
5. Validate with cloud CI processes
6. Enable code owners to sign-off on changes or enhancements

Pull Requests can even be used across different repositories.

For example, developers could easily create a PR to merge code from repo1/feature/fix-ui-login ⮕ repo2/develop.

What is the secret weapon to great Pull Requests? Rebasing

Best-Practice Rules for PRs

1. Create a Pull Request

Video: Creating a Git PR

When creating a Pull Request here are the guidelines:

• Code changes/work must be in a Feature branch
• Avoid large quantities of changes in a single PR
• Squash the commits to 1-3 commits
• Commits must conform to commit conventions
  • If UI changes, provide Before/After images in message body
  • must contain Refs #<xxx> or Fixes #<xxx> footer notes
• Rebase is required (using git pull --rebase origin <destination-branch>)
  • squash before rebase to reduce conflict issues
• Local repo passing prettier, build, lint, and tests checks.
  • use Prettier to minimize diffs and styling issues
  • if the PR includes non-UI, business logic changes
    • should have 1..n unit tests
    • may need 1..n integration tests
  • if the PR includes UX changes,
    • should have associated e2e test (if applicable)
    • should have BEFORE / AFTER snaphshots in the commit message
• The PR description should clearly articulate the PR intention and context:
  • describe what has changed or be added/redacted
  • describe how to use the feature (animated GIF is very useful)
  • for UI changes, include BEFORE/AFTER snapshots of relevant UI changes
  • include breaking API changes
  • any additional notes that will help the reviewer
• The PR must have assigned at least (1) Reviewer and (1) Code Owner
  • if changes in shared libraries, extra care is required
  • must have associated code owner approval
  • do NOT include "all" team members to review…
    • choose 1-2 reviewers
    • do not always use the same reviewer

2. WIP Pull Requests

For Pull Requests that are not yet ready for a formal review or comments, a Work-in-Progress (WIP) label should be used. A WIP PR is purposed to share ideas online or demonstrate interim progress with a non-trivial change.

WIP Pull Requests:

• should only have WIP if the PR is used to preview or share ideas
• should be considered NOT READY for review
  • developers should NOT comment on WIP unless solicited by the authorauthor
  • these PRs are consider incomplete may fail any or all tests

3. Open Pull Requests

Video: Using WIP PRs

Guidelines

• Use labels (needs-review, ready-for-merge, etc)
• Can only be merged by SEL (or designatee)
• Must pass CI tests/lints before ready for merge
  • should not have it.only(), fdescribe(), fit(), xdescribe(), or xit() tests
  • should limit # of disabled tests
• Should have recent rebasing from <destination> branch
• Should have at least one code review
  • must have code review approval before assigning LGTM labels
  • should expect the code review to be performed in < 24 hours
• For urgent PRs, the reviews and authors should be notified regarding the urgency.
• Should follow documented Code-Review practices

4. Approving Open PRs

• see the section below for Code-Review best practices
• should be marked with LGTM by Reviewer and Code owner
• should pass all tests
• if the PR contains multiple commits
  • squash commits into 1 commit or logic set of commits
  • for clarity, multiple commits are squashed into logical groups

5. Merging Approved PR

Video: Merging PRs

Guidelines

• should NEVER use the default 3-way merge. Always use the fast-forward merge
• always maintain a linear commit history
• archive/delete associated feature/fix branch after merge
• merge (after rebase) with ff-only; keep commit timeline FLAT

6. PR Cleanup

• delete the origin/feature/<xxxx> branch
  • Note: This can be done automatically via repo settings post-merge

# To delete the remote branch `thomas/conversation`, use
git push :thomas/conversation