Picture of Peter Smith
Refinements to the PR/Merge process (notes from the core developer meeting part 1)
by Peter Smith - Monday, 21 December 2015, 2:02 PM
 

The core framework developers met in Brighton (in the UK) on the 9th December. It was a really productive day with good wide-ranging debate on a number of items and plenty of actions for all of us. I will write a couple of posts summarising the discussions and their outcomes. This first one deals with the main reason for holding the meeting; refining our process for updating the core framework.

The main discussion concerned the need to balance the ability to be able to issue regular releases with minimal administrative overhead with the need to ensure that changes are adequately reviewed by the right people.

It was agreed that:

  • Only core team members who actively participate on the project should be allowed to merge
  • All changes should be associated with a ticket/GitHub issue describing why the change is required/what the issue is and how to replicate it so that we are able to verify that it is actually fixed
  • Changes should be made in a new branch spun from master in the Adapt Learning GitHub repo. This assumes that the developer has permissions to do this – if not they should fork the repo, branch and make changes there
  • Changes should be merged into master following 3 +1’s with at least two coming from core team members unless the ‘merge approver’ feels the change needs to be discussed amongst the group (either via Gitter or on the weekly stand-up meeting) or that a specific person should also +1 the change
  • At the end of each sprint there should be a code freeze for the framework and vanilla theme repos from the Monday morning to the release on Tuesday afternoon to enable a full review of the release. At this point a candidate release branch will be created from master for review so that merges can still be made into master during the code freeze
  • Following a successful review the new release will be tagged.

There is an exception to this process; three +1’s are not required for changes to README.md or example.json. Core team members can make changes to these directly; other collaborators can fork and PR with a merge approver who can check and merge straight into master.

Plugins will not be subjected to the same scrutiny as changes to the framework; core team members may merge an update with master and tag immediately.

Alongside this process change, it was proposed that the pool of people with merge rights should be reduced to 5 to ensure that any changes are stable, desirable and meet the requisite coding standards. 

Following discussion it was agreed that all people who currently have merge rights should keep these permissions, but that these rights will be reviewed on a regular basis and that people who are not participating actively in the project (ie reviewing PRs, committing changes and attending the regular stand-ups) will have these rights revoked. A maximum and minimum number of core team members with responsibility for merging will be agreed by the core team and documented.

Finally on this subject, we discussed other changes that will help to make change control easier in the future. As the biggest problems experienced to date have been caused by changes to the theme, it was agreed that moving some of the variables to core would reduce the risk of a repeat of these issues by making the core team members more aware of breaking changes. Looking further ahead, it was agreed that it would be useful to restructure Adapt into different architectural layers so that critical parts of the core code would be subject to stricter controls than other areas.

Actions:

  1. New GitHub process to be implemented with immediate effect. 
  2. The core team to document the process on the GitHub wiki and explain how to contribute, who can +1 and how code gets added.
  3. The CLI tool to be amended to default to the tagged release rather than master. 
  4. Users with merge rights to be contacted to confirm that they intend to continue to participate actively in the project and join the regular meetings. 
  5. Working group to be set up to determine which variables can and should reside in core and to make these changes. 
  6. Working group to be set up to look into making better use of Travis and automate integration, load and regression testing.