DEV Community

Mario Loncarek for Bornfight

Posted on • Originally published at bornfight.com

How to protect GitHub projects from non-reviewed code and force code review culture

In this blog post I will show you a way of protecting GitHub repositories from random pushes of non-reviewed code or pushes to the master/main branch. I’m a strong believer in deploying features via pull requests with code review. I will not get into the question if the branch and pull request workflow is good or bad, I have my opinion which is that pull requests and code reviews are a must for a team that wants to learn, so if you don't agree this article is not for you because it forces all team members to use it.

Code review is a practice that depends on the culture. A culture with no ego, with a great wish for continuous learning, sharing and teamwork. Besides learning code, code reviews will improve your communication soft skills as you will need to be clear and professional without being harsh which will also show you if you are a good mentor or not.

This article will focus on 3 steps to achieve pull request with code review practice:

  • Workflow in theory
  • Setting up your project
  • Creating pull request template

Workflow in theory

1.On GitHub create a new branch from master and use standard naming convention for branches:

  • feature/name-of-the-feature
  • fix/name-of-the-fix

Try to do this for every feature/fix to avoid creating very large pull requests which will be very time consuming for reviewers.

2.After you finish working, commit and push your code to your feature/fix branch and create pull request to merge this branch to master branch.

3.Assign someone for code review. The goal here is to learn from each other and to make sure all standards are met and code style is respected, and of course to make sure code does not have any bugs.

4.If there are any questions/suggestions/fixes/changes requested from the person which is conducting code review, that person will request changes on GitHub with clear comments and the process starts again. All comments need to be resolved before the reviewer can accept the pull request, either changes or only answering the questions.

5.After successful code review, the branch will be merged into the master branch and the feature/fix branch will be automatically deleted.

Setting up your project

1. Create CODEOWNERS file

Code owners file defines individuals or teams that are responsible for code in a repository. Code owners are automatically requested for review when someone opens a pull request that modifies code that they own. To use a CODEOWNERS file, create a new file called CODEOWNERS in the root, docs/, or .github/ directory of the repository, in the branch where you'd like to add the code owners. I’m a simple guy so I always put everything in the root. You can assign different code owners for different branches. In practice, no one could approve pull requests without code owners approval. This will protect developers from trying to merge pull requests that no one approved.

Example CODEOWNERS file:

* @marioloncarek
Enter fullscreen mode Exit fullscreen mode

2. Manage user roles

On GitHub repository go to the Settings tab and then choose Manage access from the left menu. Here you can define which users can access your repository and what are their roles. Always have at least one administrator, and all other developers can have write permissions. In practice, administrators could override everything from this article and use their privileges to make changes on master branch or force merge without review. This could be helpful for hotfixes.

Alt Text

3. Configure branch protection settings

On GitHub repository go to the Settings tab and then choose Branches from the left menu. Under Branch protection rules click Add rule.

Alt Text

This will open branch protection configuration. Under Branch name pattern write your main branch name (probably master) or any other branch you wish to protect. Configure all options according to this image:

Alt Text

This configuration will:

  • require pull request reviews before merging
  • require review from code owners
  • restrict who can dismiss pull request reviews
  • require status checks to pass before merging
  • require branches to be up to date before merging
  • require conversation resolution before merging
  • restrict who can push to matching branches
  • disallow force pushes for all users with push access
  • disallow users with push access to delete matching branches

Creating pull_request_template.md file

When you add a pull request template to your repository, project contributors will automatically see the template's contents in the pull request body.

To make your pull request template visible in the repository's root directory, name the pull request template pull_request_template.md and put it in the root of the repository.

Now when a contributor creates a new pull request they will see the template which will make pull request more standardised and contributors can be reminded about stuff that is important for the project (like standards, code style, build flows etc etc) with checklists.

Example pull_request_template.md file:

## Description

Please include a summary of the change or which issue is fixed.

## Type of change

- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

## Area of change

- [ ] Frontend
- [ ] Backend

## General checklist:

- [ ] My code follows the style guidelines of this project
- [ ] I ran `npm run format`/`yarn format` before commit
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation where needed
- [ ] My changes generate no new warnings
- [ ] I have checked my code and corrected any misspellings
- [ ] I have updated `master` and merged to my branch before submitting pull request
- [ ] My pull request generate no conflicts with `master` branch
- [ ] I requested code review from other team members

## Frontend checklist:

- [ ] I followed guidelines for `HTML`/`LIQUID`, `SCSS`, `JAVASCRIPT` from readme
- [ ] My `Javascript` generate no new console errors  
- [ ] I tested my code cross browsers
- [ ] My slice is pixel perfect for both desktop and mobile according to design
- [ ] I conducted basic QA to assure all features are working
- [ ] I tested responsive for mobile and tablet resolutions

## Backend checklist:

- [ ] I tested admin by manually adding content from zero
- [ ] I followed guidelines for naming admin fields
- [ ] I created easy to use admin experience which is self-explanatory
- [ ] I added description to admin fields in hard-to-understand areas
- [ ] I followed guidelines for naming `php`/`liquid` variables
- [ ] I conducted basic QA to assure all features are working
Enter fullscreen mode Exit fullscreen mode

Conclusion

These 3 setup steps will create strong protection of repositories against pushes of non-reviewed code or direct pushes to the master/main branch. It will force the team to apply the rules and maintain the code review culture.

It’s not always easy to enforce people to follow the rules, but with suggestions given throughout the post, you can automate them quickly, to make sure that everyone follows them.

What do you think about rules mentioned above? Do you have other advices to improve your team’s workflow?

Discussion (3)

Collapse
difilippoagustin profile image
difilippoagustin

Hi! Thanks for the article! In the last company where I worked, we created branches according to the user story and its sub-tasks, for example: the US's name was US-1234 then one sub-task was, i don't know, add a new title, so we created two branches, one for the US called US-1234 and other called US-1234__add-new-title. So you worked in the branch related to the sub-task and when the code was ready you did a merge request to the principal branch, US-1234. I hope it was understood, i don't have a good level in English.

Collapse
adamgordonbell profile image
Adam Gordon Bell

That makes sense to me. It does sound like a lot of merging though.

Collapse
difilippoagustin profile image
difilippoagustin

Yes, a lot of them.