Bitbucket Code Review Process using Pull Requests

Posted by: Rod Morison 6 years, 10 months ago

(0 comments)

Contents

Introduction

In this article I walk through a sample Bitbucket setup for a code review process using pull request and branch permission features. While all the nuts and bolts are there for a solid code-test-review-merge process, I find the documentation very nonlinear and more of a reference than a how-to. Hence, this article.

In this walkthrough, I'll use Mercurial, but everything here applies quite uniformly to Git on Bitbucket. (Github does not support Mercurial :-/.) I'll assume basic familiarity with cloning, committing, pushing, and branching. It's dealing with the Bitbucket site we'll focus on.

First, let's look at a visual of the process we're setting up.

Dev & Devtest is the developer's code activity, up to the point of committing code to the repo. A developer may make one or many commits before requesting a review. Then, when ready, the developer bundles up all work to date for review. If the review is accepted, the commits are merged into an upstream repo, where they may be built or deployed.

A review often produces a request for one or more changes, suggestions for improvements, or possibly even, "Start over from scratch" or "Abandon this line of development". Bitbucket's commenting, tasking and approve/deny features support these.

In webdev environments, we would likely deploy approved code to a dev or stage server. A "next level" of process would include integration test, acceptance test, etc., with eventual deploy "into the wild". While that's not covered in this article, we will mention where test automation would fit into the process using the build status feature. Platform projects would typically automate compile, link, and unit test steps into an analogous process.

And finally, this how-to is less of a hands-on walkthrough and more of a sequence of screenshots and explanation.

Hey Ho, Let's Go!

I'm going to use two separate Bitbucket accounts. The first will be the admin/reviewer/branch-sheriff, who doles out permissions and does reviews. The second will be a humble developer, submitting code for review, rework, and eventual merge for build...once the necessary approvals are received.

Note, in the Bitbucket model, all code reviewers are treated equal. While the admin can set a "require approval from N reviewers" criteria, one cannot say, "require 2 approvals, and one has to be code-master Yoda". The closest you can get to that is by setting a "Default reviewers", who are notified whenever a new request is submitted. They can then jump on the request, or hang back and let other staff enjoy the fun.

I've not found this to be a limitation. But, if your process calls for it, another level of merge indirection can be setup for a "blessed by Yoda" process. I like the simpler model, as code-master Yoda's tend to get backed up, and generally would delegate anyway. As the feature is, they have the option of reviewing directly, standing back completely, or using a non-review channel (email, chat, ...) to interact. Plus, the peer review practice builds team responsibility, fault tolerance in reviewers, and cherry pie a la mode (it's just that good!) In any case, I digress.

First, some

Preliminaries

Bitbucket has some nice "meta" features for managing groups of repos and groups of people accessing those repos: Projects and Teams. Though they're not absolutely required, I'll set them up anyway, as they come in handy in the real world of revolving team members and evolving code bases. (If you're actually playing along at home, you'll want to login with that admin Bitbucket account now.)

The Teams menu gets you to the "Create a Team" form:

With that done, let's create a project from the Projects menu:

Setup a Repo

And, finally, let's create a code repository:

We'll call it "catwseb" for our new website. In this demo, we'll make it a public repo (uncheck "This is a private repository".) We'll use Mercurial as the "Repository type", but as mentioned, everything here works, albeit untested, with Git repos.

We'll populate the repo from the command line, even though we could create dummy "scm-test" files via the web interface, just to be one-hundred-percent. Glossing a few details, like make sure your ssh pubkey is loaded into your bitbucket account, we fire up a local shell and

mkdir -p ~/projects/cats
cd ~/projects/cats
hg clone ssh://hg@bitbucket.org/herding-cats/catsweb
cd catsweb/
curl http://getbootstrap.com/examples/starter-template/ --output index.html
hg add index.html 
hg commit -m 'genesis'
hg push

Take a visit to the repo on Bitbucket, and you should see something like

This repo will be our deploy/build repository. Now, lets fork a "dev repo". From the catsweb repo page click on the ellipses in the left ribbon and choose Fork, like so:

In the fork form, make sure you're using the same Owner and Project as the original, and enter a new name, typically oldname-dev, or perhaps oldname-nameofdeveloper. We use catsweb-dev. Of course, we're not private here (avoids having to pay for any of this!) and we fork at the "tip" of the source repo (or whatever that is in Git.) No need for advanced settings.

The next step is not required. Not required, if you have a dev team in perfect communication between leads and devs, with devs that follow process flawlessly, without fail. (Heh.) Seriously, here's how you "lock this down" and give it enough teeth to make devs use it, whether they like it or not. As previously mentioned, approval rules are pretty flexible, not overly onerous, imo. It's called "Branch permissions".

Before we do that, we need to bring our developer onto the team. When you setup your team, Bitbucket created two permission groups for you: administrators and developers. Often, that's enough. You can edit team members and groups from the team page, click "Manage team" in the upper right. This link warps you direct to the "User groups" of the team settings. Click the "Developers" link then add them using their Bitbucket username or email.

Bear in mind, the Developer access group has write access to all repos of the team. (Projects handle no access control; they're just grouping a convenience for repos.) As is, there's nothing stopping a developer from pushing commits straight in to catsweb, unfettered. That's not ideal if you want a policy driven process. Therefore...

Hellooo, branch permissions! Go back to the original repo, catsweb, not the forked copy, click on the gear icon (bottom of left ribbon) and in Settings choose "Branch permissions", then click "Add branch permission".

In the branch permission form, leave the "Branch or pattern" at default; for now, that's the only branch in play. Give team admins heading-cats:administrators full write access. This selection will lock herding-cats:developers out of direct commits to default branch of the repo, while preserving full write access for administrators.

Give both groups "Merge via pull request". We're going to allow developers to merge their pull requests (from the -dev repo)...under certain conditions. Here is where you might apply a different rule, and limit pull request merges to administrators, another "senior dev" group you could create.

Question: but developers can still get code in to the catsweb repo unreviewed? Answer: click on "Add merge checks". (At this time merge checks are "free premium".) Finally, here are the merge policy options: We can require 1 or more approved reviews and some number of successful builds. (I won't tackle "successful builds" setup here.) I enable the "Reset approvals..." and "Require all tasks..." options, which will be shown later in this walkthrough.

Review Some Code

Alright, we're all setup for the second contributor, we'll call them "developer", to add code for review. Of course, we want them to clone the catsweb-dev repo. This repo will receive their commits, and when ready, those commits will be submitted for review, and merge into the original catsweb repo. Let's do it "100", and clone from the command line, edit, and push.

We'll "simulate" editing a file by applying a patch. From the developer account:

mkdir -p projects/cats
cd projects/cats
hg clone ssh://hg@bitbucket.org/herding-cats/catsweb-dev
cd catsweb-dev/
# email index.html
patch -p1 <<EOF
--- a/index.html    Sun Nov 06 17:20:32 2016 -0800
+++ b/index.html    Sun Nov 06 18:37:45 2016 -0800
@@ -12,17 +12,17 @@
     <title>Starter Template for Bootstrap</title>

     <!-- Bootstrap core CSS -->
-    <link href="../../dist/css/bootstrap.min.css" rel="stylesheet">
+    <link href="http://getbootstrap.com/dist/css/bootstrap.min.css" rel="stylesheet">

     <!-- IE10 viewport hack for Surface/desktop Windows 8 bug -->
-    <link href="../../assets/css/ie10-viewport-bug-workaround.css" rel="stylesheet">
+    <link href="http://getbootstrap.com/assets/css/ie10-viewport-bug-workaround.css" rel="stylesheet">

     <!-- Custom styles for this template -->
     <link href="starter-template.css" rel="stylesheet">

     <!-- Just for debugging purposes. Don't actually copy these 2 lines! -->
     <!--[if lt IE 9]><script src="../../assets/js/ie8-responsive-file-warning.js"></script><![endif]-->
-    <script src="../../assets/js/ie-emulation-modes-warning.js"></script>
+    <script src="http://getbootstrap.com/assets/js/ie-emulation-modes-warning.js"></script>

     <!-- HTML5 shim and Respond.js for IE8 support of HTML5 elements and media queries -->
     <!--[if lt IE 9]>
@@ -69,8 +69,8 @@
     <!-- Placed at the end of the document so the pages load faster -->
     <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script>
     <script>window.jQuery || document.write('<script src="../../assets/js/vendor/jquery.min.js"><\/script>')</script>
-    <script src="../../dist/js/bootstrap.min.js"></script>
+    <script src="http://getbootstrap.com/dist/js/bootstrap.min.js"></script>
     <!-- IE10 viewport hack for Surface/desktop Windows 8 bug -->
-    <script src="../../assets/js/ie10-viewport-bug-workaround.js"></script>
+    <script src="http://getbootstrap.com/assets/js/ie10-viewport-bug-workaround.js"></script>
   </body>
 </html>
EOF
hg commit -m 'fix links with absolute hrefs'
hg push

Just for fun, let's see what happens if this developer tries to push directly to the catsweb repo.

hg push ssh://hg@bitbucket.org/herding-cats/catsweb

pushing to ssh://hg@bitbucket.org/herding-cats/catsweb
running ssh -C hg@bitbucket.org 'hg -R herding-cats/catsweb serve --stdio'
searching for changes
1 changesets found
remote: Warning: Permanently added the RSA host key for IP address '104.192.143.3' to the list of known hosts.
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: permission denied to update branch default
remote: transaction abort!
remote: rollback completed
remote: abort: pretxnchangegroup.bb_perm hook failed

There you have it. The merge checks are in effect. So, as the developer, lets hop over to bitbucket.org and create a pull request for our commit. From the catsweb-dev repo, click "Pull Requests" from the left ribbon. As the screen suggests, "Create a pull request".

Almost everything in this form can be used as-is: the pull request is from catsweb-dev and to catsweb; it's pulled the commit message as the title of the pull request, that's fine. We can add additional commentary to be picked up by the reviewer in the "Description". But, this is gud nuff, so just hit "Create pull request".

Only thing is, no one was notified about this pull request! Couple of ways to address this issue. First, any team member can subscribe to pull request notifications from the "magic eye" icon in the top right of the repo overview page.

That's fine, and probably a good idea for the team's lead developer, but there's a more pointed way to request a review. Again, as the developer edit your pull request and add to the Reviewers input.

Now, when I save that edit to the pull request, my reviewer will get an email (and other integrations are possible), something like:

The reviewer can use links in the email or work directly from the website and review the commit(s). There are a number of features in this form, e.g., a general comment, line specific comments, ...; note however that after submission, the pull request "lives" in the target repo, not the repo it originated from, if you're looking for it.

One not so obvious feature is that you once you've created a review comment, you can highlight some text, and click "Create task" under the comment. Remember, when we setup the merge checks above, that bit about "Require all tasks to be resolved"? Well, here's where that comes in. You can create a little to-do list for the author, that needs to be checked off before a pull request merge will be allowed.

Up in the top right of the pull request review page, you probably noticed the "Decline" and "Approve" buttons. First, what happens if we hit decline?

Note, that if we decline a pull request, that "kills it"; it cannot be edited and resubmitted. Decline really means, "No way, not touching this...ever." Of course, a submitter could rework and resubmit. However, in most cases, what you really want is to request changes, review them again, and then approve, after one or more cycles. You've been warned about "Decline", though.

Append a Request

Let's say the developer submits a pull request, and the reviewer comes back with something like

That is, a task, "Please add favicon.ico". So, as the developer, we have this task to complete before the pull request can be merged. As developer, we add that file, push it to Bitbucket, and head in to the repo pull request page to tick off the task.

And guess what? The commit pushed is already added to the pull request ("Commits" tab of "Edit" pull request.) Note that "auto-add" is branch specific, i.e., will only be added to a pull request associated with that branch. Auto-add branch specific, but not contributor specific; a different committer to the same branch will have their work added. This behavior is exactly what you want, particularly for any git/hg flow kind of process.

Finally, the reviewer can "Approve" the pull request. You're probably curious: can I approve my own request? Sure...but, it won't "count" towards the required # of approvers.

And the author can merge their pull request into the "deploy/build" catsweb repo:

There are some potential complexities at this point. pull request behind target: use "Sync now":

What if a file changes in the target repo while a pull request is pending. Something like the following (resolving conflicts, yuck!)

Success!

We made it! We got through the whole life cycle of the process in the first figure.

Does it Scale?

That worked well for one developer and one reviewer. How does this process and tool scale to real world projects? Pretty well, and there is more than one way to do it.

One Dev Repo, Multiple Branches

The simplest approach is to stick with the single "-dev" repo and develop on feature branches. This works well for small groups, and dovetails into Gitflow or Hgflow, if working with a process like that. You might have noticed in our pull request create that there was a branch dropdown for both the PR source and destination. That, along with the "Close branchnamehere after the pull request is merged" gives just what you need for either of the xflows.

Note that in our walkthrough we couldn't close the main branch (default or master). It's the main branch, right? When using the "Close after merge" model I like all my PRs to go from the "-dev" repo feature branch in to the upstream default branch. That way, the merger is forced to deal with merge conflicts immediately. It's possible to do your PR merges into the same branch in the upstream, then merge those in to the main. That can become problematic if merges stack up, but it's a possibility.

Multiple Repos

Multiple repos is what I consider "old school", before branch level PRs became available. It's still the norm for open source, where anyone can fork your repo. A multi-repo approach is completely compatible with branches, feature or otherwise; just another level of indirection and separation. Even for more tightly controlled projects, that might make sense for, e.g., for an outsource team that's going to deliver major sub-components to spec, for a systems integration stage of a project.

References

Here are a few references into Bitbucket documentation:

• Review and discuss a pull request

• Work with pull requests

• Add default reviewers to pull requests

https://confluence.atlassian.com/bitbucketserver/keeping-forks-synchronized-776639961.html

hg commit -m 'Fix: add missing css template'
curl http://getbootstrap.com/examples/starter-template/starter-template.css --output starter-template.css

• Tags:
• code review
• software quality
• agile

• ← Python 3 dictview ... dict().keys, .values(), .items() are dynamic
• Largest palindrome product from projecteuler.net →