This blog is part of a blog series, so you can find the first page here (https://blogs.sap.com/2023/02/02/sap-cpi-ci-cd-from-from-zero-to-hero/). This is the agenda we’re following:
Code Review is most likely your last quality gate before releasing and transporting your developments to the next environments, so I don't need to convince you on the importance that such step has on the quality of your interfaces. Despite we do many of our checks automatically via CPI Lint saving time to our reviewers, there are things that were not yet automated via CPI lint rules due to the lack of time. There are also topics that can't be automated easily and need some common sense and manual checking.
Since day one that we have code review, but initially it was a quite informal (despite mandatory) task. A developer would invite another one, explain briefly the code steps, the reviewer could ask some questions or provide some inputs, but in the end, almost always the code was reviewed without major changes. If that sounds suspicious to you, it also sounded suspicious to me... After some reflections I got into some conclusions on why I consider this a flawed approach:
We had a brainstorming session about this, and based on previous reviews plus the experience of the group we came with a check list of mandatory checks to review (this would solve step 2 above).
Another outcome was that we would invest on a code review process that would be transversal to both our teams (CPI team using Gitea and Biztalk team using TFS).
There are many code review tools available in the market, some of them integrated into git itself, meaning you could do a pull request in order to merge your changes into the main branch and with that a code review is done to validate if your branch can be merged into main stable branch. After thinking about using pull requests as a code review approach, we saw no real benefit since we wouldn't do anything with the main branch after integrating the pull request. Our synchronization from SAP Cloud Integration (CI) to GIT only happens in that specific direction, we don't synchronize back from GIT to SAP CI because that would bring additional problems to solve (locks, conflicts, no more single source of truth, etc).
Someone from the team suggested Fisheye+Crucible and after an evaluation and a POC, everyone enjoyed it and we adopted it for both teams.
This is a tool to provide searching and diff capabilities to your git repository from the crucible+fisheye dashboard. It works by synchronizing your git repository into a local fisheye repository allowing you to have a single central place for all your source code repositories (Gitea+TFS)
This is the tool to do the code review process itself. It has the concept of projects that would serve as aggregators (you create a review for a project). You could group code that would make sense to review together coming from different sources. We would like to avoid this complexity, so we make it very transparent by having one fisheye repository for each SAP CI package and one project for each SAP CI package and consequently, the review process is by SAP CI package as well. You can have multiple code reviews for each project.
Below you can find a high level diagram on the steps being done nowadays in an automated way
This step was already part of the pipeline and is responsible to load the CI package source code when the pipeline runs (currently daily) into our git server.
AFAIK, Fisheye needs a synced copy of your git repository represented as a local repository used by fisheye when doing the searches that is also used for the code review processes. We create a repository per SAP CI package. Only the commits that got synced are visible on your review. We create this local fisheye+crucible repository if it doesn't exist (via crucible api) also supplying the necessary authorization (we're using ssh key) to connect to gitea.
After having a local fisheye repo, we can force an incremental synchronization (delta). This will force Fisheye+Crucible to fetch the latest changes committed to git synchronizing into it's own local repository. Jenkins pipeline waits until this process is finished
As stated above, we create a crucible project per SAP CI package to keep simplicity since our reviews apply to SAP CI package and not to a higher level. If such crucible project doesn't yet exist we create a new one on the fly. Upon creation, you can configure the owner (we take the identified package responsible and we consider that one as the owner). As the list of reviewers, the whole SAP CI integration team is allowed to review.
We check for existing reviews for the package we're processing. If they exist in Draft or in Review, we add the latest git changes there. If no open review exist but we have git files that were changed and committed for the package, we open a new code review in Draft mode.
Finally, we add these unreviewed files to either an existing or a newly created review. Then during our daily calls we ask for a colleague to do the review and the reviewer is manually assigned by the owner to the automatically opened review. The review can then take place by the reviewer (he needs to check the code against the code review check list defined) and then he/she can add comments (even on a line level).
When it's time to release the SAP CI package to transport it to the next environments, we manually check that all code reviews for all the packages we want to transport are closed (meaning that there are no unreviewed files).
We never have unreviewed files that aren't part of a code review session. Even if you change one file today, and you don't review it, if you would come back in one year to change another file, the one you didn't yet review would still be there for you to review. This would mean that we make sure that every change in any file of your cpi package is acknowledged and reviewed by someone at some point in time. All this process is managed in an automated way with no efforts for the developer or the reviewer (they don't need to know all steps described, only that crucible is maintained up to date with the git source)
In this topic, I’ve introduced the topic of code review, the challenges we faced and how the whole process evolved over time.
I would also invite you to share some feedback or thoughts on the comments sections. I’m sure there are still improvement or ideas for new rules that would benefit the whole community. You can always get more information about cloud integration on the topic page for the product.
With this post we complete the blog series. You might wonder that this approach deviates a bit from what you're usually used to see on a CI/CD approach, for instance automatically deploying tested committed code into other environments. In fact, as we don't want to synchronize from git back to CI (for the reasons I described above), despite technically possible, it doesn't make much sense to deploy it automatically from git. We prefer to have that control on our side evaluating when the deployments are done to which environments and therefore we trigger the transports and deployments manually after aligning with the functional teams (but by using the Jenkins pipelines specified on the "Release Management" page).
I hope you enjoyed and got inspired to try some of these ideas on your current customer/employer. Please let me know if you need some guidance and I'll try to help.
- Backup Binaries and Source Code
- Code inspection and quality control
- Release management
- Certificates expiration
- Automated Testing
- Documentation
- Code Review (explained in this page)
Code Review
Code Review is most likely your last quality gate before releasing and transporting your developments to the next environments, so I don't need to convince you on the importance that such step has on the quality of your interfaces. Despite we do many of our checks automatically via CPI Lint saving time to our reviewers, there are things that were not yet automated via CPI lint rules due to the lack of time. There are also topics that can't be automated easily and need some common sense and manual checking.
Taken from https://www.reddit.com/r/ProgrammerHumor/comments/lxk09d/code_reviews/
Code Review - How it started
Since day one that we have code review, but initially it was a quite informal (despite mandatory) task. A developer would invite another one, explain briefly the code steps, the reviewer could ask some questions or provide some inputs, but in the end, almost always the code was reviewed without major changes. If that sounds suspicious to you, it also sounded suspicious to me... After some reflections I got into some conclusions on why I consider this a flawed approach:
- Code was being reviewed on the fly, with little buffer for reviewer intervention or even the proper time to digest and think about alternatives
- There was no standard check list in place on what to review, so it was really up to the reviewer to have the imagination to think about problems or scenarios with the code being reviewed
- Despite there's no proof, I highly suspect that some reviewers got intimidated by providing feedback on the spot to the producer of the code
We had a brainstorming session about this, and based on previous reviews plus the experience of the group we came with a check list of mandatory checks to review (this would solve step 2 above).
Another outcome was that we would invest on a code review process that would be transversal to both our teams (CPI team using Gitea and Biztalk team using TFS).
Code Review - How it evolved
There are many code review tools available in the market, some of them integrated into git itself, meaning you could do a pull request in order to merge your changes into the main branch and with that a code review is done to validate if your branch can be merged into main stable branch. After thinking about using pull requests as a code review approach, we saw no real benefit since we wouldn't do anything with the main branch after integrating the pull request. Our synchronization from SAP Cloud Integration (CI) to GIT only happens in that specific direction, we don't synchronize back from GIT to SAP CI because that would bring additional problems to solve (locks, conflicts, no more single source of truth, etc).
Someone from the team suggested Fisheye+Crucible and after an evaluation and a POC, everyone enjoyed it and we adopted it for both teams.
Short intro to Fisheye
This is a tool to provide searching and diff capabilities to your git repository from the crucible+fisheye dashboard. It works by synchronizing your git repository into a local fisheye repository allowing you to have a single central place for all your source code repositories (Gitea+TFS)
List of synced repositories view on fisheye+crucible
Short intro to Crucible
This is the tool to do the code review process itself. It has the concept of projects that would serve as aggregators (you create a review for a project). You could group code that would make sense to review together coming from different sources. We would like to avoid this complexity, so we make it very transparent by having one fisheye repository for each SAP CI package and one project for each SAP CI package and consequently, the review process is by SAP CI package as well. You can have multiple code reviews for each project.
Crucible project with list of reviews
Code review automated process steps
Below you can find a high level diagram on the steps being done nowadays in an automated way
Code review automated process steps
1. Read package source and 2. Commit to git
This step was already part of the pipeline and is responsible to load the CI package source code when the pipeline runs (currently daily) into our git server.
3. Create Fisheye repository
AFAIK, Fisheye needs a synced copy of your git repository represented as a local repository used by fisheye when doing the searches that is also used for the code review processes. We create a repository per SAP CI package. Only the commits that got synced are visible on your review. We create this local fisheye+crucible repository if it doesn't exist (via crucible api) also supplying the necessary authorization (we're using ssh key) to connect to gitea.
4. and 4a. Trigger Git->Fisheye sync
After having a local fisheye repo, we can force an incremental synchronization (delta). This will force Fisheye+Crucible to fetch the latest changes committed to git synchronizing into it's own local repository. Jenkins pipeline waits until this process is finished
Code review showing changes on the latest commits
5. Create crucible project
As stated above, we create a crucible project per SAP CI package to keep simplicity since our reviews apply to SAP CI package and not to a higher level. If such crucible project doesn't yet exist we create a new one on the fly. Upon creation, you can configure the owner (we take the identified package responsible and we consider that one as the owner). As the list of reviewers, the whole SAP CI integration team is allowed to review.
Project definition example
6. Create crucible review
We check for existing reviews for the package we're processing. If they exist in Draft or in Review, we add the latest git changes there. If no open review exist but we have git files that were changed and committed for the package, we open a new code review in Draft mode.
Review details screen example
7. Add committed files to the review
Finally, we add these unreviewed files to either an existing or a newly created review. Then during our daily calls we ask for a colleague to do the review and the reviewer is manually assigned by the owner to the automatically opened review. The review can then take place by the reviewer (he needs to check the code against the code review check list defined) and then he/she can add comments (even on a line level).
Review comments
When it's time to release the SAP CI package to transport it to the next environments, we manually check that all code reviews for all the packages we want to transport are closed (meaning that there are no unreviewed files).
Value added
We never have unreviewed files that aren't part of a code review session. Even if you change one file today, and you don't review it, if you would come back in one year to change another file, the one you didn't yet review would still be there for you to review. This would mean that we make sure that every change in any file of your cpi package is acknowledged and reviewed by someone at some point in time. All this process is managed in an automated way with no efforts for the developer or the reviewer (they don't need to know all steps described, only that crucible is maintained up to date with the git source)
Next steps
- Integrate crucible into the transport pipeline to make sure that there are no code reviews in draft or in review for the packages you want to transport
- Check a possible integration with JIRA and collect the reviewer from the JIRA User Story (US) automatically as well as showing on JIRA US the reviews associated with
Summary
In this topic, I’ve introduced the topic of code review, the challenges we faced and how the whole process evolved over time.
I would also invite you to share some feedback or thoughts on the comments sections. I’m sure there are still improvement or ideas for new rules that would benefit the whole community. You can always get more information about cloud integration on the topic page for the product.
Conclusion
With this post we complete the blog series. You might wonder that this approach deviates a bit from what you're usually used to see on a CI/CD approach, for instance automatically deploying tested committed code into other environments. In fact, as we don't want to synchronize from git back to CI (for the reasons I described above), despite technically possible, it doesn't make much sense to deploy it automatically from git. We prefer to have that control on our side evaluating when the deployments are done to which environments and therefore we trigger the transports and deployments manually after aligning with the functional teams (but by using the Jenkins pipelines specified on the "Release Management" page).
I hope you enjoyed and got inspired to try some of these ideas on your current customer/employer. Please let me know if you need some guidance and I'll try to help.
- SAP Managed Tags:
3 Comments
