Integration Review

Revision as of 00:29, 11 May 2015 by Eloy Lafuente (stronk7) (talk | contribs) (On-syn period)

Jump to: navigation, search


The purpose of the integration review is to:

  • Ensure consistent quality across the codebase
  • Ensure that pedagogical aims of Moodle are at the forefront of any change
  • Take into consideration the holistic view of moodle, looking at the impact beyond where the original developer was focused
  • Provide guidance and feedback to developers, helping them learn and improve
  • Consider other perspectives of other users perhaps not considered by original developers e.g.
    • Teachers
    • Students
    • Administrators
    • Third-party developers

Integration Review Process

  1. Run automated pre-checks against the continuous integration server. (In future this can be automated and also moved into publicly accessible domain.)
  2. Final code review, much like the peer review, except this is the final check. To include
    1. Takes place in-situ (integrated) to examine the impact against other integrated issues
    2. Checking against the coding guidelines - syntax/whitespace
    3. Moodleisms - using the built-in api functions where appropiate
    4. Cross-DB compatibility
    5. Security
  3. Check purpose - the bug needs to fix the issue reported.
  4. Ensure backwards compatibility is is maintained. As a starting point backwards compatibility must always be maintained. Where backwards compatibility is affected it should be:
    1. Well discussed with evidence of justification
    2. Documented and communicated to the community
  5. Check the right people have been involved (e.g. component maintainers)
  6. For fundamental changes, check that a thread has been started in an appropriate forum, and other Moodlers given enough time to comment.
  7. Tests - must be written to guide tester to verify the fix is working.
    1. Unit test - very much preferred if applicable
    2. at end of wednesday, ensure testing is going to complete as expected. else take other actions (speak to test manager)
  8. Performance - we have to look at maintaining optimum code here, as far as simple patches that can affect performance. (simple optimisations)
  9. Scalability - if master only - we're looking far future , stable branches may not be lucky to get such improvements..
  10. git authorship is correct vs committer + credits due are mentioned + email addresses
  11. Documentation / phpdoc / readability
  12. Tracker_issue_labels which might need adding. Particularly:
    1. docs_required / dev_docs_required / release_notes: About which type of documentation is required for the issue.
    2. ui_change / api_change: About the implications of the change.
    3. unit_test_required / acceptance_test_required / qa_test_required: About the need to cover the issue with some test.
  13. Fixed Version after integration - is the versions that the issue is patched for. (A rule here : mention master if its the only branched fixed, if not don't mention master)
  14. Update the workflow counters
    1. If an issue is in need of a second review +1 to errors column in integration workflow counters

Integration Principles

Integration (non-technical but philosophical) principles (4-5 words determining if something has to be integrated/backported or no):

  1. safety: if something does not look safe, stable, it won't land. Be conservative.
  2. security: all security issues, if not breaking principle (1) will be integrated/backported to all security-supported versions.
  3. community: Anything not useful for the community (or against it) won't be integrated/backported. We can measure the community as 10%HQ, 10%Partners, 10%Core developers, 20%Admins, 20%Teachers, 30%Students - not exact science, just one approximation, you know). Question yourself how the change will affect those groups and ensure positives are bigger always (only affected groups count). All community issues, if not breaking principles (1) and (2) will be integrated.
  4. typology: bug fixes will be always integrated/backported to all the supported braches if none of the principles (1), (2) and (3) are violated. Also, partially-unsupported branches can receive some if they are important enough. Improvements and new features go, exclusively, to master only, that's the main reason for short release periods. We *must* not make exceptions to this.
  5. priority: issues will be "ordered down" by priority down (where priority is a mix of various factors, dynamic). And will be integrated in that order. If something has to be delayed, better if it is low priority. Once again, nothing here can break any of the previous principles.
  6. tests: unit tests and acceptance tests will backported as far as possible without breaking (1) and (2). New features required to implement tests will be backported if the API is 100% backwards compatible.

If all the principles are fulfilled, the answer for "should I integrate this?" is, "yes, please!" (apart from technical findings, of course, that can lead to the issue being not integrated/reopened at last, these principles are 100% philosophical)


In normal periods

The integrators adhere to the following schedule:

  • Sunday 22:00 UTC: Issues waiting for integration brought into current integration.
  • Monday: Integration
  • Tuesday: Integration
  • Wednesday: Testing. Integrators duties during this time are to monitor, facilitate and 'problem solve' the testing process.
  • Thursday: Testing should be completed by 07:00 UTC, at which time remaining testing failures will be reverted and reopened. The release process follows.
  • Friday: Should be kept free from integration. Integration systems are maintained during this time.

During continuous integration/Freeze/QA period

During the continuous integration period the integration team are continuously focused on producing regular builds of master to facilitate QA and fast fixes to issues identified.

  • Throughout:
    • Issues are picked on a one by one basis, prioritising QA blockers and master regressions (MUST FIX) issues.
    • Any non bug fix issues are given the integration_held label and are explicitly not picked for integration unless unblocked by the Lead Developer
    • Our goal is to achieve 'releaseability' throughout, so we stop integrating to ensure a release happens
  • Monday: Integration and testing happens.
  • Tuesday: Integration happens until 12:00 (UTC+8), afterwards we try to achieve 100% 'Test Passed' and stop integrating any untested changes until a master release is produced.
  • Wednesday: [Assuming a master release has been rolled] Integration and testing continues
  • Thursday: Integration and testing continues
  • Friday: Integration happens until 12:00 (UTC+8), afterwards we try to achieve 100% 'Test Passed' and stop integrating any untested changes until a master release is produced.

Note: along this period we always release as many stable weeklies as master rolls (on-demand, beta, rc) happen (see MDLSITE-3470).

On-sync period

Immediately after a major release and for a short period (right now, 3 weeks, matching HQ sprint duration), the integration team is under the named on-sync period.

At all effects, it's a normal period (see above), and weeklies are produced for supported stable branches and also for mater. But with one important rule:

  • We must keep the latest stable branch and master 100% on-sync, specifically about versions and upgrade steps.

This simple, but important constraint, is there to facilitate the integration of impeding bugs, needing urgent resolution, and by keeping them the same, we guarantee that any stable or master fix will apply without problems to both branches. Of course, in order to achieve the rule, these must be also observed along the period:

  • We continuously perform diffs between the latest stable and master, controlling that we are on-sync. Any non-authorised difference is cleaned (rewritten).
  • Both improvements and new features (and, in general, everything leading to divergence) are held until the on-syn period ends.


Whilst we'd all like all Moodle users to be using our latest and greatest code, there is a balance to strike between improving our software and maintaining stability (both in terms of regressions, but also training and documentation materials). Large amounts of change on the stable branches make the lives difficult for institutions to manage upgrades between point releases.

General policy

Our general policy is as follows:

  • Bug fixes will be backported to all supported stable branches.
    • When fixing a bug, please provide a fix for all supported stable branches.
    • If a fix doesn't make sense to be backported to every branch, please make it clear in the issue.
  • Improvements or new features will only land in master.

Process for requesting a non bug-fix backport

Improvements or new features can be requested to be backported to the stable branches. We urge developers to consider this request carefully. In recent years, Moodle has moved to a short and predicatable time based release schedule and we use a very effective distributed source control system. Both of these process changes should ensure that a change not being backported to the stable branches is not as problematic as it may have used to be.

Should you feel that a new feature or improvement needs backporting, please follow this process:

  1. File a new issue.
  2. Set the issue title using our backport template guide. (i.e. "Fix forum alignment (backport of MDL-99999)") - see Tracker_guide
  3. Link the original issue
  4. You should include clear rationale for the request to backport

The integration team will process backport requests, with the following guidelines:

  1. The integration team will together consider each request individually considering the needs of the community (influenced by forum posts, moodle partners, nagging developers etc).
  2. Backports will happen not earlier than 3 weeks and not later than 2 months after the issue has landed in master.
  3. Rationale will be given for rejection

If the backport request is approved, please follow the usual development process to submit the feature or improvement on earlier branches.

Polite note about bug classification

Many issues can be appropiately classified as borderline bugfix/improvements. We politely request that developers do not try and 'game the system' by clasifying their improvements as bugs intentionally. If your fix is in a grey area, please state your case for it being a bug fix clearly. The integration team will use their discretion where necessary.