Skip to Content
Author's profile photo Former Member

Code Review with Phabricator – an open source, software engineering platform

*** If you just want to skip to the good stuff here’s the github project with a saplink nugget: lucastetreault/zPhabricator · GitHub



Phabricator is a collection of open source web applications that help software companies build better software. It is developed and maintained by Facebook and largely based on their own internal tools.

/wp-content/uploads/2014/11/hero_592759.png

The major components of Phabricator are:

  • Differential – a code review tool
  • Diffusion – a repository browser
  • Maniphest – a bug tracker
  • Phriction – a wiki

My company has a very mature Java Development team and they have been using Phabricator’s Differential for their code review process for quite some time. Our ABAP team has experimented with various processes for enabling and enforcing code reviews and I recently hacked together some integration so that we can use Phabricator for our ABAP code as well. Once I’ve cleaned up the code and made it easy to configure without having to change a bunch of code I’ll set up a project on github with everything you need to get up and running!

Basically the goal is to get a nice side by side comparison so that we can quickly and easily see what was changed and to be able to control the release of transport tasks by checking the status of the review. You will not be able to release tasks until the review has been accepted.

diff.PNG

***Since a couple days ago when I first put this blog up I have spent a couple late nights working on this and have managed to have the whole process inside of SAP. I’ll leave the “old new” process at the bottom of this blog so you can see where I started…

Here is what our NEW process looks like:

You are done coding in a transport task and you are ready to have it reviewed and moved up to the test tier.

transport.PNG

Go to transaction ZPHAB and enter the details required to create a code review:

zphab.PNG

Run the program and a revision will be created:

zphab result.PNG

Browse to the link it gives you and you can view the revision:

review.PNG

If you make more changes to the transport task (ie: if your reviewer requested changes) you can use ZPHAB again and it will update the revision with the latest changes.

If you forgot to submit a task for review you will get the following error when you try to release it:

/wp-content/uploads/2014/11/7_592773.png


If you submitted the task for review but it hasn’t been approved you will get the following message including the URL of the revision in Phabricator so you can send it off to someone to follow up if needed.

/wp-content/uploads/2014/11/8_592774.png

**** This is the “old new” process before I did a bunch of work to have the whole process in SAP.

Here is what our (old)new process looks like:


You are done coding in a transport task and you are ready to have it reviewed and moved up to the test tier.

/wp-content/uploads/2014/11/1_592769.png

Open a command prompt and navigate to C:\Phabricator. When you get there, run the command sapdiff <task number>

/wp-content/uploads/2014/11/2_592770.png


You’ll get a popup of notepad++ and you’ll need to enter a few details:

/wp-content/uploads/2014/11/3_592772.png


<<Replace this line with your Revision Title>> — This needs to be set to the task number that you are submitting for review

Reviewers – Enter the phabricator user name of the person (or people) you want to review this

/wp-content/uploads/2014/11/4_592771.png


Save the file, then close notepad++. The differences have now been pushed to Phabricator. You will receive an email confirming that a ‘revision’ has been opened and the reviewers will receive an email asking them to review your code.



If you forgot to submit a task for review you will get the following error when you try to release it:

/wp-content/uploads/2014/11/7_592773.png


If you submitted the task for review but it hasn’t been approved you will get the following message including the URL of the revision in Phabricator so you can send it off to someone to follow up if needed.

/wp-content/uploads/2014/11/8_592774.png


I’m pretty excited about this! It’s a HUGE improvement over our old process. Hope you like it!


– Lucas


Assigned Tags

      8 Comments
      You must be Logged on to comment or reply to a post.
      Author's profile photo Frank Vieregge
      Frank Vieregge

      Hi Lucas,

      couldn't the comparison of the object be done in the standard version history?

      Thanks for sharing! I remember you implemented the Fitnesse plug in for ABAP as well.

      Thanks,

      Frank

      Author's profile photo Former Member
      Former Member
      Blog Post Author

      Hey Frank,

      That's how we used to do our code reviews. We used an internal message board where we would give a transport number then whoever was reviewing would have to login to that system and drive in to each individual object and pull up the version history. It definitely works but it's just very manual and time consuming. We also have multiple development environments so we'd be login in to various systems to review code.

      Moving to Phabricator means we have 1 consolidated place where we can easily view all the changes from a transport task. There are also some great built-in tools for collaboration such as inline commenting in the review, adding reviewers and you get all kinds of email alerting + cool stuff for free by using Phabricator.

      - Lucas

      Author's profile photo Lars Hvam
      Lars Hvam

      Hi, thanks for the blog.

      Can you tell a bit more about the sapdiff program? I guess it is custom code, and perhaps gets the new abap code via HTTP?

      It looks like there is an unstable json API for Phabricator, so would it be possible to submit the code review task directly from ABAP to Phabricator via HTTP?

      Author's profile photo Former Member
      Former Member
      Blog Post Author

      Hey Lars,

      I wasn't kidding when I said I 'hacked together' some integration.

      In SAP I have exposed an RFC function module which determines the objects included in a transport task and then uses the FM SVRS_GET_VERSION to get the ABAP text for any versionable objects that it can. I then wrote a Java program using the SAPJCO to call the RFC FM to get the version from the Dev system and the Test system. The java program then invokes a command line program to generate the 'unified diff' that Phabricator expects. When that's done I use Phabricator's Arcanist command line tool to create the revision in Phabricator. 'sapdiff' is just a batch program that runs all these steps 🙂

      All of this could be accomplished directly in SAP if I can figure out a way to create the unified diff in SAP. I looked at the GNU implementation of it last night and it's about 1400 lines of C. I'm not sure if I have the time to port it to ABAP in the near future.

      I am using Phabricator's REST APIs to check the status of a revision in the 'check_before_release' BADI so if I can figure out the unified diff it will be trivial to create the revisions from SAP.

      Author's profile photo Lars Hvam
      Lars Hvam

      cool, thanks

      I also need a longest common subsequence implementation in ABAP for one of my projects, will let you know if I get time to look into it.

      Author's profile photo Sven Kotting
      Sven Kotting

      Hi Lucas,

      thank you for sharing. I am very interested in optimizing our enviroment since I am pretty sure that time is wasted and knowledge lost in our dev departement.. No dev myself just ops, I may miss a good piece of how-to do it better and what is necessary for our devs. But certainly will try and talk them into it to improve.

      Keep on sharing.

      Sven

      Author's profile photo Matthew Billingham
      Matthew Billingham

      In a couple of places, we've manipulated the standard process, so that as the developer releases his task (and he can't release the full transport), a popup asks for a reviewer. A review task is then created on the transport. The reviewer can accept or reject the development (with reasons). If the development is rejected, a new task for the developer is created. In any case, the review task is released.

      So you can't release a transport unless it has been reviewed and accepted.

      But... as a Java developer, I appreciate what you've done. It's great.

      Author's profile photo Bar P
      Bar P

      Hey Matthew, I know this post is from 2014 and its 2019 now, but can you elaborate on how you've done those things? I'm interested in adding a way for our team to do code reviews.

      And if you have other solutions now than 5 years ago I would greatly appreciate hearing them 🙂

       

      -BarP