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
eduardoc 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)
Story time....
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.
Benefits:
- 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.
Challenges:
- 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.
- Slowness
- 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
abapGit modifications:
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.
Finally...
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.
