On Tue, Jun 2, 2015 at 11:39 AM, Colleen Murphy <coll...@gazlene.net> wrote:
> In today's meeting we discussed implementing a policy for whether and when > core reviewers should abandon old patches whose author's were inactive. > (This doesn't apply to authors that want to abandon their own changes, only > for core reviewers to abandon other people's changes.) There are a few > things we could do here, with potential policy drafts for the wiki: > > 1) Never abandon > > ``` > Our policy is to never abandon changes except for our own. > ``` > > The sentiment here is that an old change in the queue isn't really hurting > anything by just sitting there, and it is more visible if someone else > wants to pick up the change. > > 2) Manually abandon after N months/weeks changes that have a -2 or were > fixed in a different patch > > ``` > If a change is submitted and given a -1, and subsequently the author > becomes unresponsive for a few weeks, reviewers should leave reminder > comments on the review or attempt to contact the original author via IRC or > email. If the change is easy to fix, anyone should feel welcome to check > out the change and resubmit it using the same change ID to preserve > original authorship. Core reviewers will not abandon such a change. > > If a change is submitted and given a -2, or it otherwise becomes clear > that the change can not make it in (for example, if an alternate change was > chosen to solve the problem), and the author has been unresponsive for at > least 3 months, a core reviewer should abandon the change. > ``` > > Core reviewers can click the abandon button only on old patches that are > definitely never going to make it in. This approach has the advantage that > it is easier for contributors to find changes and fix them up, even if the > change is very old. > > 3) Manually abandon after N months/weeks changes that have a -1 that was > never responded to > > ``` > If a change is submitted and given a -1, and subsequently the author > becomes unresponsive for a few weeks, reviewers should leave reminder > comments on the review or attempt to contact the original author via IRC or > email. If the change is easy to fix, anyone should feel welcome to check > out the change and resubmit it using the same change ID to preserve > original authorship. If the author is unresponsive for at least 3 months > and no one else takes over the patch, core reviewers can abandon the patch, > leaving a detailed note about how the change can be restored. > > If a change is submitted and given a -2, or it otherwise becomes clear > that the change can not make it in (for example, if an alternate change was > chosen to solve the problem), and the author has been unresponsive for at > least 3 months, a core reviewer should abandon the change. > ``` > > Core reviewers can click the abandon button on changes that no one has > shown an interest in in N months/weeks, leaving a message about how to > restore the change if the author wants to come back to it. Puppet Labs does > this for its module pull requests, setting N at 1 month. > > 4) Auto-abandon after N months/weeks if patch has a -1 or -2 > > ``` > If a change is given a -2 and the author has been unresponsive for at > least 3 months, a script will automatically abandon the change, leaving a > message about how the author can restore the change and attempt to resolve > the -2 with the reviewer who left it. > ``` > > We would use a tool like this one[1] to automatically abandon changes > meeting a certain criteria. We would have to decide whether we want to only > auto-abandon changes with -2's or go as far as to auto-abandon those with > -1's. The policy proposal above assumes -2. The tool would leave a canned > message about how to restore the change. > > > Option 1 has the problem of leaving clutter around, which the discussion > today seeks to solve. > > Option 3 leaves the possibility that a change that is mostly good becomes > abandoned, making it harder for someone to find and restore it. > > I don't think option 4 is necessary because there are not an overwhelming > number of old changes (I count 9 that are currently over six months old). > In working through old changes a few months ago I found that many of them > are easy to fix up to remove a -1, and auto-abandoning removes the ability > for a human to make that call. Moreover, if a patch has a procedural -2 > that ought to be lifted after some point, auto-abandonment has the > potential to accidentally throw out a change that was intended to be kept > (though presumably the core reviewer who left the -2 would notice the > abandonment and restore it if that was the case). > > I am in favor of option 2. I think setting N to be 3 months or 6 months is > appropriate. I don't have very strong feelings about options 1 or 3. I'm > against option 4. > > Colleen > > [1] > https://github.com/openstack/nova/blob/master/tools/abandon_old_reviews.sh > Though we didn't seem to reach a clear consensus in this thread, based on feedback from today's IRC meeting I have added a section based on option #2 to the wiki[1], with an addendum that this policy is subject to change. We can try this out for a few months, and if we find that we have a problematic number of old patches that aren't being addressed, we can revisit the policy. I think having some kind of policy in place that can be updated is more important than the specifics of the policy. Colleen https://wiki.openstack.org/wiki/Puppet#Patch_abandonment_policy
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
