Menu
«

Git & Pull Requests

All our projects use Git with a repository hosted on GitHub.

All changes should be merged via Pull Requests (PR), with the exception for certain projects where merging between develop and master can be done directly via the command line (for those who have access to do this).

Branches

Once a project's gone live, the master branch must always be stable. It should be safe to deploy the master branch to production at all times. All branches are assumed to be active, stale branches should get cleaned up accordingly. The develop branch (if any) will generally hold commits for the next release.

Some projects have two branches: master and develop. Avoid committing directly on the master branch, always commit through develop via PRs.

If the project has a develop branch, all PRs except hot fixes, should be made to develop (which should always be the base branch).

Naming

Branches may only contain lowercase letters, slashes and hyphens.

Examples:

Commits

There's no strict ruling on commits, however, descriptive commit messages are recommended. Always use present tense in commit messages.

Ideally, prefer granular commits.

For larger PRs which will contain multiple commits, it's ok to use wip commits to save progress. To keep a clean history the PR must then be merged using the Squash & merge option.

Opening a Pull Request

Make sure you open the PR to the correct branch (develop if it exists and not a hotfix).

Write a good description and link to the relevant task.

The title should follow the same rules as for commits.

Reviewers

Add at least one reviewer. If you don't care who reviews your code, add multiple people that have a good. knowledge of the project. This means the PR is "up for grabs".

If you want multiple people to review your code, add all of them and be specific about that in the description.

Labels

In Github we have a set of "repository defaults". These are added to all new project automatically since they are set as our "Repository labels" for our Github organization. See https://github.com/organizations/adaptivemedia/settings/repository-defaults for more information.

These are our current labels:

Most often when you open a PR, you should add the labels Needs review and Needs testing.

It is a good practise to open a PR with a WIP label as early as you can, so the branch is not forgotten and so others can look at it and make suggestions early in the process.

These labels are not set in stone, and you can add more labels if needed to a project.

Reviewing a Pull Request

Make relevant comments where needed. Try to be positive. If everything seems ok, Approve the PR and remove the Needs review label. If the PR doesn't need testing, you can add the Ready for merge label.

If something needs to be changed or addresses, Request changes and add the label Action needed.

If the PR is approved and can be merged, BUT something small needs attention (but not further review), add both labels Ready for merge and Action needed and specify what needs to be done in the comments.

Merging a Pull Request

When a PR has been approved and has the label Ready for merge, it's ready to be merged into the target branch. Usually the author of the PR is the one who merges it.

Generally we use the Squash & merge option if you have lots of small commits (eg. you forgot to rebase them locally or you just fancy "wip" commits), otherwise use Create a merge commit.

When merging from develop to master via a PR, always use Create a merge commit because then we will have one single commit for each release. This also means you must merge master back into develop manually to make them equal.

Large Pull Requests

To maintain efficiency and manageability in our development process, we strongly advise against creating very large Pull Requests (PRs). A useful guideline is to consider a PR excessively large if it involves changes to more than 40 files.

Instead, we recommend dividing larger tasks into smaller, more manageable PRs. Ideally, each PR should correspond to a distinct subtask. Moreover, to minimize integration complexities, each PR should be merged and deployed as swiftly as possible. This approach helps avoid scenarios where a primary PR becomes the base for numerous dependent sub-PRs, leading to challenges in testing, addressing issues in the base PR, and ultimately in merging these PRs.

It's important to highlight that this is a general guideline, and the actual number of files or the size of a PR may vary depending on the project and the nature of the changes. One PR could change e.g. 60 files, but if the changes are small and isolated (e.g. search and replace fixes), it could still be manageable.

Sometimes it's more efficient to create a larger PR than having 4 separate PRs in a chain. Use your best judgment and discuss with your team if you're unsure.

The main concern is to keep the PRs manageable and easy to review. It's very easy to miss things in a large PR, and it's also harder to review and understand the changes.

For some cases, a new feature could be hidden behind a feature flag or a similar mechanism. This enables us to develop a feature in smaller steps and PRs since each PR can be deployed since the code will not run if the feature is disabled. It's important to notice however that there is a complexity of handling feature flags that may not be worth it for smaller features.

PRs that split up backend and frontend changes

When a PR involves changes to both backend and frontend code, it is often a good idea to split the PR into two separate PRs (one for the backend and one for the frontend). Usually, the backend PR is done be a backend/fullstack developer, and the frontend PR is done by a frontend/fullstack developer.

The benefits are:

  1. Avoiding the PR to become too big
  2. Easier for both the developers to work on isolated branches.
  3. Easier for all reviewers to review a small PR.
  4. The backend developer is probably the one creating the PR and also reviewing the frontend changes, which makes it confusing to review his/hers (own) PR.

When doing this, both PRs should be reviewed independently and approved before either of them is merged to make it easier for the reviewer. When the Frontend PR is merged, the Backend PR should be tested and a final review can be done before merging it.

Resources