Integrating a bug-fix into Moodle core - the mechanics
Having seen Tim’s post about fixing a bug in Moodle core I thought I should take the opportunity to follow up with the next part of the process - integrating the changes into the Moodle core. Once a developer has produced a fix which is ready for integration and requests a integration review then the bug is ‘handed over’ to the integration team.
The integration team
The integration team is staffed by a group of developers at Moodle HQ. We are a geographically disparate team, spread across the world in order to prevent natural disasters and political unrest from affecting the Moodle weekly release cycle. ((That might not be the reason.. ;-) )) Aparup and I are located in Perth, Australia; Eloy in Lardero, Spain and Sam in Nelson, New Zealand.
Together we have 20 years of Moodle development experience and this collective experience is an important part of how we work together as a team - knowing the idiosyncrasies of Moodle - how parts of the system have come to being and understanding how changes can potentially affect different areas which might otherwise be missed. I should say that since i’m the newest member of this team (and still learning the ropes) I hope that my comments are accurate!
The integration review
The integration team is responsible for ‘pulling’ code from all developers around the world, reviewing and bringing it together (integrating it!) to make up the Moodle core release. This process is the same for all developers no matter whether they work for Moodle HQ, a University, Moodle Partner or none of the above.
The purpose of the integration review could be misconceived simply as as a final technical code review before code gets into core. That is certainly a large part of it - we are striving to continually increase the quality of Moodle releases through technical reviews, increased testing and other processes. But the integration review is also about examining the impact of each change from our users point of view - ensuring that we don’t loose focus of Moodle as supporting education! Other aspects of the integration review include ensuring that discussion has taken place with interested parties before a change lands and facilitating that if it hasn’t as well as providing guidance and feedback to contributors - helping developers learn why we might want changes or if there is something that could be useful for next time.
Anyway enough preamble, the title promised mechanics so..
Down to mechanics
At the start of each week we being a new integration cycle and on average there are 30-40 issues waiting for integration into Moodle core. The process begins with an integrator moving issues waiting for integration into the ‘current integration’. Issues marked as in the ‘current integration’ are those which we will work on for this cycle. We make this distinction simply to avoid the bucket of issues we are working to integrate for the weekly release from constantly refilling. The integrators work through the queue of issues waiting for integration one by one - in this case I cheated a bit and allocated myself MDL-32039 early, as I thought it’d be a good opportunity to do a follow up post to Tim.
Reviewing the History
The first step is to mark an issue as ‘in integration review’ in the tracker workflow (pictured) and starting to read back the comments in the issue - learning about the history of this change. In this case the change is fairly obvious and straight forward so there is not too much to consider. But sometimes there might be debate about a particular approach or how to solve an issue and comments from peer reviewers or watchers of the issue. Or indeed there might be no discussion at all when someone is proposing do something particularly radical which is equally important to note. As a brief side note - I personally encourage all developers to ‘vocalise’ even their internal though process through bug comments and commit messages. Its an often forgoten about communication method but comments on a bug like this can help reviewers now understand the decisions without too much back and forth. And perhaps more importantly months and years down the line it can be used by other developers to understand why a change was introduced even if you are no longer working on Moodle or if you’ve forgotten.
Reviewing the code
At the point of integration review, we review changes ‘in situation’ to see how the fit with other integrated issues. So the first step is to pull the change into the master integration branch.
danp@marge ~/git/integration> git checkout master
Switched to branch 'master'
danp@marge ~/git/integration> git fetch origin
danp@marge ~/git/integration> git reset --hard origin/master
HEAD is now at abd2899 MDL-31768 - it is not possible to add a picture to the thanks page in feedback
Some notes about this first step. On git.moodle.org we have two repositories, the production moodle.git (which almost everyone should be using) is the result of what the integration process produces. We also have integration.git which is our ‘staging’ repository where we pull changes, integrate and test - this is where integration work happens before deploying to moodle.git. integration.git is intentionally separate and volatile, this is where we iron out the kinks before pushing to the production moodle.git. I have a separate integration checkout which I am using here for reviewing changes and pushing to the integration repository. To explain what I did - I first checked out the master branch of the integration repository and brutally reset this to the current state of the integration repository to accept new changes. In most git workflows this is not an advisable strategy but the way I use my integration repository I always want it to be completely clean and blow away any changes (in fact I have a script which does this on all branches) so I am happy to do this. Now to pull Tim’s changes:
danp@marge ~/git/integration> git pull git://github.com/timhunt/moodle.git MDL-32039
From git://github.com/timhunt/moodle
* branch MDL-32039 -> FETCH_HEAD
Merge made by the 'recursive' strategy.
.../lang/en/tool_qeupgradehelper.php | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
I now have Tim’s changes in the master integration branch and it merged successfully without issue. This was straight forward - which is not surprising as its a fairly self contained issue and nobody else has been working on that language file this week. But simple merges are not always the case. We ask developers to rebase their changes against the latest weekly release to reduce the frequency of conflicts, but sometimes the integrator will have two changes integrated during the same week and there will be conflicts. Common examples of merge conflicts are multiple changes to the major version number in one week. This is where integrators need to examine the differences and either resolve the conflict manually or delay an issue as it might be affected by another issue which is conflicting. Since that was straight forward I can move straight onto reviewing the change:
danp@marge ~/git/integration> git diff origin/master..master --stat
.../lang/en/tool_qeupgradehelper.php | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
danp@marge ~/git/integration> git diff origin/master..master
First I look at a quick summary of lines changed - this is useful to find if there are any unexpected places where lines are changed. Then I look at a diff in unified format - which when reviewing open source code you get very used to. But since this is a language change I actually decide to review it in a more readable format by using the –word-diff option of git diff.
danp@marge ~/git/integration> git diff origin/master..master --word-diff
In –word-diff format we are much more easily able to see this change and it looks good. In other circumstances the –ignore-space-change option can also be useful to find just the changes that have happened ignoring space changes such as indentations (when introducing a conditional and so forth). We will also look around the files in full form and history of file changes around the area in question. I tend to use the command line and vim, others will use IDEs such as netbeans and eclipse.
Commence pre-flight checks
At this stage I have reviewed the change, tested it and understood the impact and i’m confident in the code change that i’m ready to integrate the change. However, we have another tool up our sleeves for checking changes before integrating - our loyal servant Jenkins. Jenkins is a continuous integration server which we are in the early stages of integrating into the Moodle workflow thanks to some great work by Eloy.
I am able to run a pre-check on Tims branch before integrating. This pre-check will run the various code checkers against Tim’s change to spot problems and then produce a report on this. In the future its planned that this will happen without an integrators involvement automatically when an issue is submitted for integration. Luckily (and as expected) the checks ran successfully and no issues were found so I am now ready to push to integration.
Rinse and repeat
Since the change in question affects multiple branches and not just master I need to repeat the review process for all other branches required (2.1 and 2.2 in this case). In the interests of keeping you awake, i’ve spared you this process. :)
Pushing.. done [1]
The time has come to push to the integration repository:
danp@marge ~/git/integration> git push integration-push master MOODLE_22_STABLE MOODLE_21_STABLE
Compressing objects: 100% (24/24), done.
Writing objects: 100% (26/26), 3.14 KiB, done
21cadef..7d7331b MOODLE_21_STABLE -> MOODLE_21_STABLE
18ca35b..41b4bb0 MOODLE_22_STABLE -> MOODLE_22_STABLE
abd2899..6461e14 master -> master
Done? Well not quite.. at this point the previously mentioned continuous integration server notices the change in the integration repository which has been pushed and starts running a series of tests on the changed branches. There are too many checks to go into here and they continually being evolved, but for example we will run all unit tests and ensure ’the build is not broken’ as well as conduct fresh installs and upgrades and compare that upgraded data is exactly matching newly installed data. This automated testing stage can prevent serious and difficult to spot regressions before they make their way into production and indeed in last weeks integration cycle it caught two such bugs.
Mark as Integrated
Once the change has been pushed, we will then mark the change as integrated in the tracker and the worflow will change to mark the issue as ready for testing.
Unresolved
Since this blog post has grown to much longer than I expected, I will stop my description of the integration process as this point. I hope some of you made it to the end :)
[1] The title is a little in-joke as we integrators have a simple semaphore system to help avoid unnecessary conflicts by first calling out ‘pushing’ (on a chat channel) when starting the process of pushing branches and releasing ‘our lock’ completing with another shout of ‘done’.