How we do ABAP code review using abapGit
We need a way to do code review for our ABAP code, and we decided to give abapGit a go.
We have been looking for ways to enforce code review before the transport can be released in our on premise ABAP systems. We can’t find a standard SAP way, but the rest of the world seems to happily use git. Hence we started looking at how to use the notorious abapGit git client for this purpose. We’ve come across a blog by Eduardo Copat that suggested this idea, and this blog is about how we tried to productionise the approach with a few twists.
Our understanding is that abapGit is intended for a different purpose, therefore we expected some compatibility issues with our requirements.
At a very high level, what we have done is use abapGit to serialize our entire code base in production and push it to our internal corporate Github repository. We then create a new repository in abapGit in our development environment that points to the same github repository. (They will be out of sync showing lots of differences but it doesn’t really matter)
A developer completes development and is ready to release their transport. He releases all the subtasks of a transport (the transport is not yet released). At this point we have some custom code to automatically create a new branch in github in the background with the same name as the transport. (this piggy backs “Transport to branch” feature in abapGit). We also added a check so that the transport cannot be released without an approved pull request.
(As the tasks are released, we intend to trigger ATC to check for any glaring problems before a code review occurs, more for another time…)
The developer logs into the github repo and can see the new branch. He will now create a pull request (PR) (from the new branch to master) and follow the usual pull request process. Approver reviews the code and approves the PR. Now the transport is allowed to be released from the system. Note that we block the git merge from happening until later, as the transport is code reviewed but not yet released.
If the approver asks for a change in the code, then the changes should be added to the same transport, and when the developer releases the subtasks again, the branch and the PR is updated with the changes.
When the transport is released/imported to the target system, the new branch is then merged automatically in github to the master branch, thus ensuring github repo master branch and target ABAP system code base remains the same.
- Leverage industry standards for code review (4 eye principle)
- Every change in the transport is clearly visible in git, without the need to drill into each object in SAPGUI and compare versions
- Ensure all code is reviewed before release which fulfils our audit requirement
- Code review collaboration is captured nicely, allowing both developer and reviewer to comment at change level or even code level
- No more surprises with unintended change reaching production
- Many possibilities to integrate this with CI tool of choice.
- Notifications are out of the box for github, meaning there is no need to chase someone for a code review.
- You can’t just rely on github to review abap code
- You still need SAPGUI/Eclipse
- To fully understand the change, you might still need to go back to SAPGUI/Eclipse to fully understand the impact of the change.
- Lots of legacy code without proper package strategy
- We have a lot of legacy code. Our use of ABAP packages is not the best, resulting in logical code spread across multiple packages. The packages are big, and often code/objects are placed in the wrong packages.
- abapGit assumes one abap package can only point to one github repository (rightly so)
- Ideally you want all of the objects in your transport to belong to one package only but it is very hard to enforce. Failure to do so, the “transport to branch” feature will ignore all the objects that are not in the repository.
- To solve this problem, we have a top main parent package encapsulating all of our packages, and we sync that to github. The downside is that it created a massive package containing lots of objects.
- Enhancing abapGit code is risky
- Our changes could be overwritten easily, or contradict/incompatible with the ongoing changes in abapGit open source code after we update our abapGit code from the internet.
- Not all ABAP objects are supported by abapGit
- For objects that are not supported by abapGit, we still need to capture a review has taken place.
- As explained above our repository is massive, over 50,000+ objects, and each time we open AbapGit takes a long time as it needs to serialize everything and compare with github for changes.
- Protect this repository from abapGit
- We want to allow access to the abapGit report, but not allow anyone to temper with the settings for the code review repo.
- When doing a code review it is not easy to know about the transport dependencies
We had to make a number of changes to abapGit for this to happen. We have not yet contributed the changes back to the wider community as we are still testing if this works.
- abapGit will not let you sync a package if the parent or child package is already linked to the github repository. We had to relax this check.
- We had to develop some code to automatically create a branch with the changes in the transport
- We had to develop some code to check an approved PR exist before transport can be released
- We had to develop some code to merge an approved PR post transport release
We also had to make these changes in a way that allows us to use abapGit for code review, whilst also allowing developers to use abapGit as it is intended.
Contributing back to the abapGit open source project
We are happy to contribute back to the open source community, but we first want to get some feedback on what others think about what we are doing. It is a bit different to what abapGit was intended for. Also what we’ve changed may not be agreed by others as like I said, abapGit was designed with a different purpose in mind.
Well, hope you find this interesting, and do reach out if you need more information. I deliberately left out a lot of technical information to keep this blog simple.
I knew I read something like this before. So click on the link in the paragraph. It is an “easier” way to build this out. Or not to really build it out but use GITHUB for code reviews. Reading this new blog “just” automates it.
“JUST” meaning adding a lot of work to automate the process. I would love to see it in open source. For a large group, I can see this being a huge time saver.
I’m wondering about smaller shops – like mine. I think it is a bit daunting to even think about even putting this together. I wonder about the time vs. advantage that type of thing. Anyway would still love to see it. If it isn’t a terrible challenge to do, I think I would even try to install it and do it here.
Very cool idea!
Edited comment after response below.
Yes, there was a link to Eduardo's blog in my first paragraph. It was a little bit of work to get going at first. We are in trial at the moment and will come back later to quantify the benefits and will discuss internally around the opportunity to open source this.
Glad you liked it.
Yes, AND I read right over the top of it.
I can't wait to "see" your final benefits. It should be cool.
Hi Michael. Did you ever get the chance to share this work or happen to open source it? It's a nice approach, and I'm looking into something similar.
Nice blog 🙂
IMHO it's not risky to enhance abapGit. You can create a branch in your own abapGit fork an then rebase every now and then onto the updated master branch. That's how we do it in our shop. Of course then you have to deal/resolve the conflicts. But I wouldn't call that risky but the way to go.
Yeah, my corporate fork of abapGit carries 6 unpublished patches. The only thing I need to do is to regularly run "git pull -r" to rebase my patches with the most recent abapGit master.
abapGit is an active open source project. We've had scenarios where we changed some code, and a few months later after doing a "git pull", either the class or the method we modified disappeared. Yes, we managed to recover our changes and find another place for it, but the maintenance requires a bit of effort. Hence I described as "risky" as after an update, we don't have a lot of faith whether our solution will still work. We have yet to put some safeguarding in place. WIP.
You don't need to immediately open PRs for abapGit but you can maintain your own public fork of abapGit, so the community can have a look at your changes, approach and technical solution and even run it in their landscape (I am really interested).
BTW the benefit of git is not only the version of code but documentation of commit message. How do you enable developers releasing a transport task to describe the changes?
Sounds great. Maybe your topic should be more Something Like "How we do ABAP code review using github". Cause you are not using abapgit for review 🙂
My thoughts: If a developer releases his task you are creating a branch. But at this time its already activated in Dev system. In my opinion the Workflow should be: releasing Task, push changes to dev branch and creating a new pr to the branch of the next system (maybe qs) only containing the changes from the transport. With that you have a develop branch fitting exactly to your development system.
Thanks, nice blog, thanks for sharing
abapGit is MIT licensed, so everyone can make forks if they like. But for forks both the community and the forking party typically looses, so I recommend not making forks. There are multiple ways to handle this, but all starts with an issue on github. abapGit also offers a set of user-exits for customer specific code https://docs.abapgit.org/ref-exits.html
Also see https://github.com/abapGit/ci_tools which is meant as a set of tools for setting up scenarios like this, offering API calls to git hosts, but its work in progress
https://github.com/abapGit/background_modes uses some of the same idea as above, offering additional background modes to make one branch per transport, again work in progress
Very nice and clear summary of benefits and challenges. Thanks for the post!
Hi, Michael, great blog!
I liked your twists on the code review approach. I am particularly curious about the ATC integration that you wrote. As the other mentioned, it would be nice if you could share the code, so we can take a look ;).
Checking transport objects when transports are released is part of standard SAP ATC offering. We have not turned it on yet as we are still in the process of implementing RemoteATC.
Hi Michael, and thanks for an interesting post!
We are using a similar approach to our code review process. We are however using Azure DevOps instead of GitHub, and we are creating branches in a different way as well as handling the release of sub-tasks differently.
When the developer is done with the development and wants a colleague to do a code review, the developer uses abapGit and from the master branch does a "Transport to Branch" selecting the relevant transport request. This automatically creates a new branch in Azure DevOps and also triggers a pipeline which runs code analysis in SonarCloud as well as runs all of our ABAP unit tests. The developer then creates the pull request and the code review is performed.
If during the code review, the colleague requests changes to the code, the original developer does the changes using the same sub-task as was used for the initial development. The changes are pushed to Azure DevOps by using "Stage" in abapGit. The sub-tasks are only released after the pull request has been approved.
I didn't fully understand why you want to release the sub-tasks before the code review has been done and why you don't want to use "Transport to Branch"? Could you maybe explain a bit more in detail what the advantage to releasing the sub-tasks earlier are?
Sorry for the delayed response as we all try to survive in this crazy world....
What you do is largely the same....except we don't use Azure DevOps/SonarCloud so keen to hear from you what that's like. I see there's SonarABAP now.. how is that different to SAP ATC checks?
Now to answer your questions.
We also call the "transport to branch" functionality, but just automate it for our developers. It is quite possible (for us anyway) that the objects in a transport belong to multiple github repositories (multiple packages), and if we rely on the developer to know which repo they need to do a "transport to branch" and pull request, it could be a hit and miss. Hence we automate this process by deciding what repos are changed and do everything in the background for them.
The other reason why we automate this is because we don't really encourage devs changing the code review repository using zabapgit so we locked down the changes to a few individuals. Out of the box abapGit does not have much authorization check, and the fragile process could be broken easily. They can still use abapGit for other repositories and use abapGit as designed.
When we release our sub-tasks, we trigger RemoteATC and that will do our static code analysis. Our quality gate is at the transport request level. We make a call to github to ensure an approved pull request exist for a branch with the same name. If there are multiple repos, we do multiple checks.
Hope this answers your question.
Yes, these are strange times indeed. Thanks a lot for taking the time to elaborate on the question I had!
Now I understand why your process is different from ours. We have a single repository mirrored by a main umbrella ABAP package. So all of our "real" packages are located below this umbrella package. Due to this we don't have the issue of having to keep track of to which repository we need to do "transport to branch". A major disadvantage is however that the performance is suffering, since our monorepo is rather big.
To me Azure DevOps feels fairly similar to GitHub. There are of course differences, but I think that the basic workflow is pretty much the same. We have a lot of .NET applications and micro-services running on Azure, so I think that was the main motivation for using Azure DevOps for our ABAP repositories as well.
Regarding the ABAP checks available in SonarCloud, there is an overlap with abapOpenChecks and the standard ATC checks delivered by SAP, but there are also some checks which are only available in SonarCloud. Neither SonarCloud or abapOpenChecks completely covers the ABAP Styleguide for clean code which we try to adhere to, but they are good complements to each other.
Thanks again for an interesting post!
(Dennstedt repository brought me here! 🙂 )