Owner: User #1179751

Improvise

Attachments:

• IM

Blog posts:

• [User #1179751]: Pull Requests!

  Justifying Implementing Pull Requests on Small Teams

  IMPROVISE is a small team project, currently we have three developers and a co-op.  Nothing exciting, no need for meetings to discuss status, I just turn to User #76759 or User #76761 for any updates, and the others will overhear.  I bring this up, because the biggest complaint I've heard about implementing Pull Requests is "my team is too small to need them."  My team has been implementing them, and we all feel that this mentality is incorrect.  This post is an outline on how we are doing it between the four of us and how to setup the process in Stash.

  What is a Pull Request?

  I'm not going to argue the semantics of the verbiage here, as I think the term pull request is kind of odd, but simply put a "Pull Request" is a request to merge your branch into another branch.  IMPROVISE follows the git flow branching method described here.  While I highly recommend reading that page, I'll save you some time and sum up our branching strategy:

  • MASTER - Official releases only here, nothing else.  The only time something is merged into master is when IV&V says everything is done.
  • DEVELOP - Stable branch.  The code here can be trusted, and everything should work, it just hasn't been officially tested yet.
  • Feature branches - Come off the develop branch and belong to the developer working on that feature, once done they will be merged back into develop.

  For the purpose of this article, we are going to focus on DEVELOP and the feature branches, because that is where we implement our Pull requests.  Our workflow is pretty simple.  A developer picks the item he is going to work on (something like implement compression) and creates a feature branch off of DEVELOP.  The developer then goes and implements the changes and when complete a pull request is created to inform the team that he is done, and the changes should be merged.  This is a pull request in a nutshell.

  That sounds like process, wait one second while I grab my pitchfork.

  Full disclosure, it is process.  However, it is only a little bit of process, and it will help your team quite a bit.  We've implemented strict pull request rules on the IMPROVISE team to help with the following:

  • Avoid any "Single Bus Theory " situations.
  • Avoid single threading.
  • Produce better code.
  • Ensure time isn't wasted finding the proper commit to check out so you can work on something.
  • Allows the team lead full control over the important branches.

  I'm going to explain these all in detail, and hopefully that pitchfork will get put away.

  Protecting the "big" branches.

  I'm not a manager, so I don't need to pay attention to everything that my fellow team mates are doing with their time.  OSB is pretty big on the fabled "i-time" and we are always trying to improve things when we have a free moment.  Because of this I could care less about some of the branches that get created in my project.  If User #76760 decides he wants to see if we can gain a performance boost by switching from AESAdvanced Encryption Standard to RC6 encryption he is free to create a branch to play with this; that really is the beauty of git.  For all I, and the rest of the team cares, User #76763 can absolutely destroy the project in his branch and nothing will be an issue.  As team lead, what I do care about is MASTER and DEVELOP.  The code there is gospel in the sense that it effects everyone else's ability to work, because we treat DEVELOP as a stable branch.  In addition to outright breaking something, a fellow teammate might unknowingly implement something that makes us no longer meet a requirement (maybe the customer demanded AESAdvanced Encryption Standard).  Stash gives the ability to protect specific branches, and because of this, I am the only one who can merge code into MASTER and DEVELOP.  This gives me full knowledge of all code that will eventually make it out the door.  Pull requests make this easy by allowing for a simple click of a button.

  Avoiding "ummm, hold on a second" when branching.

  Having a stable DEVELOP branch allows for new team members to get up and running quickly.  This is because it avoids the issue of having half-finished projects on a branch that others will work off of.  We've probably all been at fault with this, where we are working on something, it is half done, but you want to commit and push it before doing more.  In this model a new developer to the project would have to search through commit messages to find a stable point to start working off of.  In the IMPROVISE project, if you want to start something, you only need to know to branch off of DEVELOP nothing else.  This point ties back into the previous one, because the pull requests help protect develop.

  Producing better code through code reviews.

  Here is the part where the pitchforks come out, code reviews can feel very "processy" but they will help in the long run.  If implemented properly, pull requests can force proper code reviews.  Stash has the ability to enforce a minimum number of reviews before code can be merged when dealing with pull requests.  On the IMPROVISE team all pull requests automatically add the other teammates on the team as reviewers.  So when User #76751 makes a pull request, User #76753 and I are notified and one of us has to approve it.  This approval process causes the perfect opportunity for a code review, and User #76752 would sit down with either User #76754 or I and talk through what the changes entail.  While this sounds a lot like what was described above in protecting DEVELOP and MASTER, I treat it slightly differently.  As team lead, before I merge something I take a cursory look at the code to ensure this fits the mission.  As an approver I sit with the developer and talk through almost every line to understand what they are doing and help spot any potential problems early.  I have everyone else on the team as approvers, because a constant string of code reviews would be too much for me to handle, and I trust my guys to be able to implement a code review properly.  So if User #76764 tells me User #76758's code is good, I'll just take a quick glance and merge it.

  Which brings us to avoiding that great big bus.

  Stash tracks who approved the code, this leaves a nice audit log for use in case a developer can't be reached.  We'll use pull request #6, which was created by me (I follow the same rules User #76755 and User #76750) and approved by User #76756.  Let's say the big yellow bus came and ran me over; User #76762 is now team lead, and has been tasked with improving the usblocker class.  If he pulls up the pull request, he will see that User #76757 approved it and can assume that I talked through the code with him.  The process of code reviews gives a team the ability to protect against the bus.

  Avoid single threading.

  This is kind of a sub point, but Stash has some nice commenting ability.  We all work different hours, and it can be hard to get on the same page.  This is shown well in pull request #9, our co-op created this request, and then had to run off to a student program thing.  User #76749 and I were able to take a look at it while he was away, and leave some comments on the request for him to address.

  A Typical Workflow Using Pull Requests

  Since you're still reading, I'm going to assume that you are at least interested.  So I'm going to walk you through what would happen if one of my team mates did some code changes to IMPROVISE.  We'll continue the example of a compression implementation.

  1. First, a branch is created based off of develop (again, we know develop contains solid code).
  2. The changes are committed to the specific branch.
  3. Development is complete, but before a pull request is generated the developer should merge develop into the feature branch to avoid any conflicts (note, this is not feature into develop, this happens later).
  4. Once all merge conflicts are resolved, create the pull request (the developer is requesting to merge the feature into develop).
  5. The pull request notifies the other developers about the pending changes.

  Now at this point, let’s assume that I am going to review said changes.  I tend to follow the typical workflow.

  1. I check out the branch and build it on my box.  This ensures there aren't dependencies that were missed.  (Hopefully this step is automatic once Bamboo is up and running).
  2. The developer and I will sit down and talk about the code changes, walk through the code line by line.
  3. We talk through unit tests (I'll chat more about these later).
  4. Approve the pull request.

  I mentioned earlier, that IMPROVISE requires a single approver, so the option to merge the changes falls to the team lead.  This process is easy:

  1. Take a quick look at the code and make sure it meets standards.
  2. Click Merge.
  3. Delete the feature branch as it is no longer needed.

  At this point, develop is now updated, and continues to be stable.  Nothing too process intensive, and in the end will ensure better code.

  Final Advice

  Some hints and tips we've discovered while doing this:

  • Keep the scope of feature branches narrow.  It is much easier to do a code review on 100 lines of code, over a couple of thousand.
  • Talk through every line of code, so a second developer understands what is going on.

   

• [User #1179751]: Trying to figure it all out.

  In the past 6 months we've received a whole bunch of fun new tools for development purposes.  Confluence, Jira, Stash, Bamboo, Dart, RainKing, Source Tree, etc; a bunch of shiny new tools, sitting in a corner collecting dust for the most part.  I'm not really sure how to use them, and more importantly I'm not sure how to use them well but, luckily, I have a big project with a small team willing to try them all out.

  In the next few months, IMPROVISE is going to be going over a pretty large overhaul.  During this process my team and I are going to keep all the new tools in mind and figure out a way to effectivley implement all the pieces into a product that will hopefully be more stable, and have a lower cost of maintanece in the future.  Keep an eye on this blog as we figure things out.

Home pages:

• IMPROVISE
• IMPROVISE Home