New CommitFest App Spec

From PostgreSQL wiki
Jump to navigationJump to search

Specification for a new CommitFest Application

The primary goals of a new CF App are as follows:

  1. to make commitfests easier, more comprehensible and more fun for reviewers
  2. to make commitfests easier for committers
  3. to make commitfests tolerable for commitfest managers, or at least to make it possible to run a large commitfest with only one CFM, instead of two

There are some secondary goals, all of which fall in the "nice to have" category, and may not show up until much later versions of the app:

  • enabling CF output to inform the release notes (currently it is too untrustworthy to do so)
  • enabling additional automated testing of new patches (some automated testing is required to make things easier for the reviewers, but we could do more)
  • making things easier and more understandable for new submitters

For compactness, the below specification references the existing CommitFest application.

General

  • All "names" in the CF application should be tied to community login accounts, with email addresses. This is to permit auto-emailing.

Yes, this means that all reviewers and patch submitters will need to have a community account.

Patches

Submitting Patches

It should be possible to submit patches using either of two methods:

  1. Submit the patch to pgsql-hackers and it gets automatcally picked up.
    • if not automatically picked up, add it manually the commitfest application.
  2. Upload the patch via the CF app, and it automatically sends an email to pgsql-hackers.

Patches which are auto-added from pgsql-hackers would be found by the following mechanism:

  • they are not replies to another message
  • they contain one or more attachments with the extension "patch"
  • they occur within 60 days of the beginning of the CF

Auto-added patches would be automatically put in the special category "unprocessed patches". This will allow the CFM to easily find them, and allocate category, author and other information.

Submitted patches should have all of the following fields. Fields marked APM below should "autopopulate from email" if they are coming from email; that is, those fields should extract the message from the archives and automatically populate, although they can be edited after population.

  • Patch name (APM)
  • Author (APM, or defaults to current user)
  • Patch Category
  • Description of Patch (only for patch upload)
  • Patch code (only for patch upload)
  • Is this a doc patch only?
  • Is this a contrib module patch? Which module?
  • Git fork link (if available)
  • Wiki page link (if available)
  • Difficult patch flag (is this a large/hard patch requiring much discussion by committers?)
  • CFM notes: a large textbox notes field, visible only to those with admin permissions, which CFMs use to communicate with each other about the patch.

Downloading Patches

It should be possible to download the latest version of a patch directly from the CF application, using a simple link. For CF items which have multiple patch files, we should track the latest version of each file.

Automated Testing

The CF application should support automated testing of submitted patches, in order to save time for both the submitter and the reviewers. This would include two levels of testing:

  1. Patch applies test
  2. Build Test

The "patch applies" test would be run on every patch submitted to the CF application. It would work by pulling down a copy of HEAD, and then trying to apply the patch to it, and recording any errors if it fails. It would be run at the following times:

  • the day after the patch is first submitted
  • every week, once a week before the CF
  • every day during the CF

The "build test" would be run only on patches where were "approved" by the CFM or someone else with admin permissions. It would be run once on each version of the patch submitted, and only once. It would involve applying the patch, then building the PostgresQL source with the patch, and running the regression tests. We would need some special automated tests for patches to contrib modules.

Patches which are "complicated", such as ones with multiple files, would not work with the automated build test. This is fine.

If possible, the build test should also be run for any new version of the patch posted on a linked thread. How to distinguish new versions is a challenge, however.

Reviews

Review Classes

(see revised concept "New States", below, since the Review Classes concept is probably not workable)

Currently we have an undifferentiated concept of "review", even though reviewers are not equally capable of doing different kinds of reviews. It would be very helpful to track the different kind of reviews which a patch needs. As such, both at review claim time, and at review posting, time, the reviewer (or CFM) should be able to post the type or types of review it is, from this list, and whether or not it passed:

  • completeness check (i.e. has docs, tests, comments, etc.)
  • functionality test (includes performance tests)
  • code review

Thus a patch might have the following review statuses:

  • completeness check - 2013-06-18 by Atri - fail - comment: no tests
  • functionality check - 2013-06-18 by Atri - pass
  • code review - not done

In general, when a patch has passed all three reviews, it's ready for commit.

Revised Concept: New States

Instead of the above, we would add to the number of linear states a patch needs to go through:

  1. Waiting on Author
  2. Needs Build Review
  3. Needs Functional Review
  4. Needs Code Review
  5. Ready for Committer

Needs Build Review should hopefully be a status only seen by our automated testing system, and then only for about a day. And, of course, at any time the patch could go back to (1) Waiting on Author.

This concept would keep the stages of patch testing completely linear (e.g. there's no functionality review if the patch doesn't build).

Review Checkoff

Additionally, we could have a checkbox set for "reviews" added to a patch, which would allow us to check off the things which had been reviewed and passed for that particular patch. It would look like:

Review Type Reviewed Passed
Make Installcheck Yes/No Yes/No
Implements Feature Yes/No Yes/No
Spec Compliant Yes/No Yes/No
Documentation Review Yes/No Yes/No
Performance Test Yes/No Yes/No
Has Good Tests Yes/No Yes/No
Code Quality Yes/No Yes/No
Code Style Yes/No Yes/No

This might be a feature left for a later revision to the app, though.

Reserving and Assigning Reviews

Reviewers may put their name next to any patch as a review they plan to do. After five days (and two reminders), this will automatically expire and their name will be taken off the review reservation on the patch.

As an advanced feature, reviewers should be able to click an "assign me a patch" button. They would then be offered a patch, which would be randomly selected among the patches which don't have a reviewer assigned and do not have the difficult flag set. They could accept that patch or ask for another one.

Submitting Reviews

It should be possible to submit reviews one of two ways:

  • via pgsql-hackers, by putting "review" in the subject line of a message on the thread where the patch was originally submitted, and likely other special fields in the body
    • using message-id if the above fails
  • via the CF application, using a webform

(Josh notes: every single new reviewer I've trained has tried to post the review in the "comments" field of the CF app. It's the intuitive route, and we should enable it.)

Additional information we'd like to have for each review:

  • reviewer (if not the person submitting it)
  • result: pass / fail / needs more review
  • review type: completeness, functionality, code (can be two or even all three)
  • review comments

UI

Searching and Sorting

The CF application should permit the following filter/search options, either alone or in combination:

  • By patch status
  • By patch category
  • By reviews passed (choose from list,including "none" or "all")
  • By author name (inc. wildcards)
  • By reviewer name (wildcards), including "has no reviewer"
  • By committer name (wildcards), including "has no committer"
  • By patch name (wildcards)
  • By greater than/less than last activity date, with "no activity in # days" search
  • By last review date
  • By last mail date (see detailed patch view below).

We should also be able to sort pending patches on the following columns:

  • Patch Status
  • Category
  • Author, Reviewer, or Committer
  • Patch Name
  • Last Activity Date (ascending or descending)

The following five dates should all be tracked by the application separately, and ultimately all be searchable"

  • Last patch update (last new email with a "patch" attachment)
  • Last review date
  • Last email thread update
  • Last status change
  • Last Activity Date (latest(last patch update, last review date, last status change))

The following special searches would save a great deal of time for the CFM and reviewers:

  • Patches without a reviewer.
  • Authors who are not listed as reviewers or committers on any patch.
  • Reviewers who have posted a review (for crediting in the release notes/wherever)
  • Committers who have committed a patch (for crediting)
  • Different patches which share the same archive link (possible duplicates)

Misc UI Stuff

Some miscellaneous notes:

  • It should not be possible for anyone other than a CFM/Administrator to add a patch to a CommitFest with any status other than "Open".
  • It should be possible for the CFM to add a patch, review, or comment on behalf of another user, and have it tagged with that user's community ID.
  • All archive links from the CF app should be to the pgsql-hackers list exclusively.
  • Patch names should be identical to the email thread which is linked by the patch (minus Re: and [Hackers]). It should not be possible for a non-CFM to change this.
  • Name and contact mailer for the current CFM and assistant CFM should be on the CF page.
  • Dates of the commitfest should be on the CF page.

As a fun refinement, we could also have a "projected end of commitfest" figure on the CF homepage. This would be calculated as follows:

   S + ( ( T * d ) / ( R + r + C ) ) * '1 day'

Where S is the start date, T is the total patches in the CF, R is # Returned, r is # rejected, C is # committed, and d is days elapsed since start date.

Notifications

Automated Notifications

All of these notifications go out by email, to the email registered with the submitters Community Account. Users should be able to opt-out of automated notifications in general, but we need not enable them to opt out of specific patch notifications.

  • Patch submitters should receive notifications (cc'd) if the status of their patch changes, including:
    • Patch added to CF
    • Patch status change
    • Reviewer added
    • Reviewer removed
    • Review posted
    • 5 days after patch is marked "Waiting on Author"
  • Reviewers should receive notification when:
    • they are added to a patch (including the date of their five-day deadline)
    • they are removed from a patch
    • the day before their deadline (4 days)
    • the day of their deadline
    • the patch status changes
    • a new patch version is posted

Additionally, when a commitfest begins for the first time, all authors should receive a notification of all of their patches in the CF. All reviewers should receive a reminder of the patches they have already signed up to review.

Manual Notifications

The CFM should be able to send out a manual notification to any of the people involved in a patch, via the app. Clicking the "manual notification" link should create an email (preferably a mailto link, but possibly a webform) which includes the following things:

  • author and reviewers of patch in the recipient list
  • subject line of "Re: Patch: title of patch"
  • body including the title of the patch and a link to the patch in the CF app.

As an additional refinement to this, there should be available several template messages:

  • reminder to review
  • reminder to update patch
  • query regarding patch status

Additionally, the author of the patch should be able to use the CF app to generate an email to the reviewers, and the reviewers should be able to use it to generate an email to the author. By default, these messages should be cc pgsql-hackers, so it's probably best to do they via a mailto.

It should also be possible for the CFM to click on any name in the CF and generate a mailto for that name. One example use for that would be to email patch authors who have not picked up any reviews.

Further, we need to be able to send blanket manual notifications to any/all of three groups, based on who's currently in the CF. These lists should be responsive to any search filters.

  • all submitters
  • all reviewers
  • all committers

Notification Templates

See the CommitFest Checklist

Stuff the CFM Currently Does which is unnecessarily Time-Consuming

All of the following tasks could be speeded up or eliminated entirely through better software, automation, or both.

  1. email reminders to reviewers, authors, and committers
  2. emailing status queries
  3. solicit submitters who aren't reviewing to review stuff
  4. find patches which aren't assigned
  5. update patch statuses based on trolling -hackers
  6. search -hackers for patches not added to the CF

Item (1) is by far the biggest unnecessary time-waster; this could be 90% automated away.