Code review for security advisory coverage applications

Given where things are and what is going on, I would like to propose the following:

We have a first draft of the code review process by DrupalCon London. This includes documentation for both applicant and reviewer. Currently, there is some specific discussion happening here. Specifically, this would include the following:

Goals

Note: The following is provided as a suggested 'checklist' for code reviews, with some modifications to the existing process thrown in.

The largest change is a divergence from the "Stick to a review from start to finish" concept, as the proposal below would encourage folks to jump on/off at the start/end of any given gate. The motivation behind this is to allow for less experienced reviewers to get involved in the review process without committing to a full review (as the 'full review' commitment can be a bit daunting for a new contributor).

A second suggestion is the use of tags to validate the clearing of each gate, as discussed in other sections of this group. While there is consensus within the group that tagging gates could become unwieldly; auto-creating tags is not something that can be done today 'out-of-the-box' ... so I've attempted to group the checklist into sections, thus minimize the tags used while still providing the abililty to use tagging to indicate where an application sits in the process.

The third is the addition of an 'Applicant Review' gate. I personally feel that the linkage between 'Module Review' and granting of the 'Full project access' roles should be severed, allowing the flexibility within the process to grant one without a dependancy on the other (think '3 module reviews = full access', or 'Module denied, but access granted' situations) ... which would then allow a 'probationary' period for new applicants, where we can ensure they are up to speed on managing the issue queue, project packaging, and git best practices before setting them free in contrib.

In any case, my proposed 'checklist' is posted below - feedback, comments, suggestions for improvement are encouraged!

While sat in the queue for my application, I thought I'd help out someone else and decided to grab a copy of their theme via Git and give it a quick once over.

Now I feel bad, as the theme @ http://drupal.org/node/1150074 appears to be a hashed copy of the Corolla theme for D7. I left a comment to that effect and suggested it would be better suited as a subtheme.

Based on my last check, the Project Applications queue is down to 134 modules currently sitting in 'Needs Review' status.

This is an amazing improvement over where it was a few weeks ago ... and a big kudos to everyone involved. However, I suspect that, without additional reviewers, the current progress will either stall out or lead to reviewer burnout ... neither of which are good options. Once the backlog has been cleared, it may be possible to stay on top of it and manage it with just a few bodies ... but first, we need to clear the backlog.

How? Well, here's a proposal ...

There's been a couple of issues raised this week which suggest we may need to have a generic discussion on module duplication and the code review process. As a result, I'm starting this thread, with the intent of 'genericizing' the discussion a bit ...

Let me start by saying that I've just fallen foul of the duplication issue in my current project review, this isn't intended as an appeal but rather a request to understand the issue more widely.

I'll explain how I duplicated and why so you can understand where I'm coming from, and then phrase a couple of questions that I have that hopefully will clear a few things I'm not quite grok'in.

The Dupe

This module has generated some discussion recently, and I feel really bad for the project owner. He went through a process of correcting things that a few reviewers pointed out to him. Then Joachim came along and said that he thought the module should be implemented as a Context plugin. The project owner basically said that he didn't want to do that and marked it 'needs review'. I found it after it had been sitting for another four weeks. I thought Joachim had abandoned the review, so I took a look at it and said I thought it was OK and should be approved.

We have several talented, experienced themers planning to attend the PDX user group's Code review/Project application meet up in a couple of weeks. I wanted to pull up a list of the project applications that were requesting review of a theme (rather than a module) as I'm getting the message that people are more comfortable reviewing in their domain, but I don't see an easy way to do that. Searching for the word theme includes lots of modules.

There are a few elements that require special skills to review. We should get folks to focus on these since someone who knows ubercart shouldn't waste their time on an issue about theme review (and vice versa).

Here are some links to issues in needs review that are for

• Ubercart
• Views
• commerce

One of the things zzolo talked about in his post was formalizing the code review process. Under that heading, he included describing the basic steps of the process and the outcomes of these steps. I put together an outline of basic steps, but I would like to get some feedback before putting it on the wiki.

Basic Steps

I've divided basic steps into two categories, based on the level of expertise required to get the job done right.

Initial Review

This is a living document on best practices around Code Review Sprints. It is not a once size fits all, so do what is best for your specific group of people.

Goals

The goal of a Code Review Sprint is usually to teach and advocate about reviewing, and work on the application queue. Though getting through applications is awesome, building long term reviewers is more important as this is a much more sustainable outcome in the long term.

We should have regular code review process reviews to go through the issues that have risen and to make changes as needed to the process.

See zzolo's post for some context.

This page is dedicated to documenting solutions around code review issues. Please be realistic about what is achievable in an amount of time.

If you just want to complain about something, please file a complaint.

See zzolo's post for more context.

Short Term

What can be done in the next week or two?

This page is dedicated to documenting issues of the code review process. These issues should be backed with data when possible and should always be objective. Please do not propose solutions here (you can do propose solutions here).

If you just want to complain about something, please file a complaint.

See zzolo's post for more context.

We need to start data collecting about the code review process so that we can make more informed decisions. See zzolo's post for more context.

We need defined leadership for the Code Reviewing. See zzolo's post for some context.

So, you think something is wrong? Good, add a comment on here to let us know. You can also help with process building and making more significant changes by going to the Process Building (see Code Review homepage).

This page is dedicated for reviewees (users that have had a review) to tell their story. If you have been through a review, comment on this page to tell your story.

We need to clearly define the goals of the Full Project Applications process (and possibly name it better). There is currently some outlined on the how to review. Also, see zzolo's post on overall way of building process.

Mission

List of project moderators

The project moderators listed below have, and use, access to grant the permission to opt projects into security advisory coverage.

• Abhishek Vishwakarma
• Alan Palazzolo
• Alberto Paderno
• ARUN AK
• Ayesh Karunaratne
• Benjamin Melançon
• Cameron Eagans