Integration Review

Jump to: navigation, search

Purpose

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
    2. ui_change
    3. api_change
    4. qa_test_required
  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)

Schedule

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.
 * With issues which have had the commits reverted in integration.git, developers should rebase their branch on top of the the new moodle.git release. Minor fixes should be logically chunked in the commits if possible.
 * If there are commits that don't change but were reverted, these can be cherry-picked to avoid rebasing issues.
  • Friday: Should be kept free from integration. Integration systems are maintained during this time.

Backporting

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. 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.