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.
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.
***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.
Go to transaction ZPHAB and enter the details required to create a code review:
Run the program and a revision will be created:
Browse to the link it gives you and you can view the revision:
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:
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.
**** 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.
Open a command prompt and navigate to C:\Phabricator. When you get there, run the command sapdiff <task number>
You’ll get a popup of notepad++ and you’ll need to enter a few details:
<<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
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:
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.
I’m pretty excited about this! It’s a HUGE improvement over our old process. Hope you like it!
– Lucas
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
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
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?
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.
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.
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
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.
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