1262172 - Document and clarify review process

Status: RESOLVED FIXED

(Taskcluster :: Services, defect, P1)

Comment 1

[Selena Deckelmann :selenamarie :selena]

Can we get this prioritized before interns start on may 23?

If there are notes to share, please dump into the bug and we can get another dev to finish this task.

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

Here's the notes that I have:

* having a defined set of review expectations for a release

  * pre-review: go through the rough design before implementation begins

  * architectural review --> clarify what the requirements are for this project. (i.e. use the existing base class) are the APIs provided the ones needed? are the APIs designed in a way that we can do what we're intending to do? other stuff: patterns used, maintainability, data structures & performance improvements.

  * example of a good review: provisioner - was doing cascading objects keyed by region and worker type; switched to having just a list of all instances/spot requests, something that can filter to give a different view

  * functional review --> what does work, does it do what we're supposed to be doing

  * making the code understandable

  * better ways to organize sections 

  * style issues

And here's the write up I did to describe the process:

Taskcluster reviews aim to ensure that code we deploy is maintainable, understandable, resilient and fast.  We’ve developed a review process that works really well for us.  It is modelled on the general Mozilla review policy.  Each component, library or service has an owner who is responsible for the overall design and implementation.  Reviews are a collaborative conversation which ensures that new code is good quality.

When a project starts, the engineers working on it figure out who the reviewer for this project will be.  It’s important that the component, library or service owner be involved in figuring out the reviewer.  This is to make sure that there’s continuity in review.  The first step of the review is to check if the overall idea of the project is good.  This might involve verifying that assumptions are correct and collecting data to show a need.

Once the design work is mostly complete and some rough sketch of the code is available, we will conduct something we call a “30% review”.  This is to catch problems with the architecture of the project and make sure that the final work is done in a way that the reviewer finds acceptable.  Every code path does not need to be fully implemented, rather it’s an overall check that the project is moving in the right direction.  If there are design problems here, it's easier to fix them than when the code is complete, which is the goal of this review.

After the code is completed, a more detailed review of function happens.  The goal of this review is to ensure that all of the code actually works as well as following style and testing conventions.  Unit tests are really useful here as they help the reviewer verify functionality of code without having to trace through it manually.  Only after all review feedback has been address to the satisfaction of the reviewer and author will we deploy the code.

Every member of the team writes code with their own style.  The purpose of our code reviews and style-checking tools is not to force a single style for everything, rather to make sure that the style used is clear, concise and readable.

Comment 3

[Selena Deckelmann :selenamarie :selena]

Thanks, John. The original intent of this bug was to put this into our docs site. Please add a page there with this content and link in the hierarchy somewhere that makes sense.

Comment 4

[[github robot]]

Commit pushed to master at https://github.com/taskcluster/taskcluster-docs

https://github.com/taskcluster/taskcluster-docs/commit/c52c1ec716abc47edd91417fcd226622e17ef02e
Bug 1262172 - Document and clarify review process