During my early years of software development I used to think of Code Reviews as a necessary bureaucratic monster, process designed to stop me from delivering the value and focus on pointing out mistakes.
My outlook at it has changed. There are many benefits of code reviews. Some of them that are more important for me:
- increases quality of code therefore improves maintenance
- facilitates sharing of the information and knowledge with fellow developers
- improves my coding skills thanks to feedback
At RBS we are using Subversion and GIT as our SVC tools. We are using Stash to manage our repositories. Stash has a very useful features that could help setup the code review as a mandatory process, before the code is merged into the main branch. In this post I would like to show you how to set it up and how to use it.
Use case for code reviews
The use case for the Mandatory code review is taken from a real case brought at my work by one of the teams. The team was typical, Technical Lead, Senior Devs and Junior Devs. They wanted to leverage the Code Review goodness for learning.
What the users wanted to do is:
- allow only specific users to be able to modify code in Master branch of GIT repository
- allow everyone else on the team to create their local branches and push those branches into remote repository
- have the ability to raise a code review of changes made on a user branch before merging the changes into the Master
- have the ability to comment, decline the changes
- once the changes were accepted to allow anyone with enough permissions to merge the code
You might notice similarity in that process to the one that is quite common in the Open Source community and was championed by GitHub, called Pull-Request (on a side note, this site is great EpicPullRequests).
Preparing repository for code reviews (or for Pull Requests)
First thing to do would be to make sure that all the people in your team are Contributors to a project. I have a group of users in Stash called superheroes. I need to set them as a Contributors on my project.
- Project level permission settings
What I’ve done above means that everyone superhero in the group would be able to contribute to the project. The next step will restrict the changes on Master branch and allow it only for a specific user (in our case, Superman).
The above action will result in only Superman being able to make any changes on Master.
What would Batman do?
For Batman (the user that is restricted on Master but allowed on Project level) to be able to work he needs to work on a branch, push that branch into Stash and create a merge request (Pull request).
Creating the Pull Request
When Batman finished working on the feature he would like to Batmobile to become mainstream and be adopted by all Superheroes. What he needs to do is to merge hist feature into the Master branch. We know already that he cannot do it as someone need to review his changes. In our case it’s the Superman.
Batman creates a Pull Request.
What Superman will see once he is logged into Stash he can review the Pull Request, approve them, decline, comment, etc.
Once the request is approved, Superman or anyone else with the permissions to modify Master can merge it.
Possibly worth to mention the fact that it is possible for anyone to review the changes as it is possible for Batman to request anyone to be the reviewer, however, only the users with enough privileges will be able to merge the changes.
Superheroes conclusion
The above setup leverages the feature of Branch Permissions in Stash. Anyone who would like for changes to be merged into the Master branch will need to go through Code Review.
Wishing you many happy reviews and much more learning.
Greg Gigon's wEbLOG 
Greg mate, that’s a fine technological solution to a basic team problem: instead of tooling up, you guys should just pair more. And that would be even more effective. Did you unlearn what you used to know?? 😉
you can make pull requests mandatory by changing the repo settings to require n number of reviews. Without this people can approve their own pull requests. (see screenshot)
is there any way to define a user group and put the name of the group in the PR?
You can define a permissions on a branch in the repository. Once this is done, only the Group that has Write permissions would be able to perform the Merger. The only requirements for the pull request that you can put is for it to be approved by at least X number of people.