RFC: Review with Flags (Version 4)

[Kevin Fenzi]

On Wed, 07 Feb 2007 23:55:16 -0500
wtogami at redhat.com (Warren Togami) wrote:

> I think this procedure should be good enough for both Mass Review and 
> general package review for an interim period, prior to a better
> design in Package Database.  I would like to ratify this process late
> Thursday if possible, so please comment soon if you see problems.

I like this plan very much, with one exception. 
(See below)

> 
> Changes since Version 3:
> ========================
> - Hybrid of "ASSIGNED to next actor" and "ASSIGNED to reviewer and
> use NEEDINFO" as summarized in 
> https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00252.html
> - Explicit description of MODIFIED and CLOSED states
> 
> Fedora Review Flag States
> =========================
> fedora-review BLANK
> 	I want a review, or a past reviewer gave up.
> fedora-review?
> 	Under Review, ASSIGNED to reviewer
> fedora-review-
> 	Denied and needs work, NEEDINFO to owner

I would very much prefer if fedora-review - flag was used when the
review was totally rejected only. Ie, the license was unacceptable, or
the submitter decided to withdraw the submission. 

> fedora-review+
> 	APPROVED, ASSIGNED to owner
> 
> Assigned Pointer
> ================
> (Note: Assigned pointer is different from the ASSIGNED state.)
> - Assigned pointer to nobody at fedoraproject.org if no reviewer yet.
> - Assigned pointer to reviewer, during the review.
> - Assigned pointer to nobody if reviewer gave up.
> - Assigned pointer to owner, after APPROVED and fedora-review+.
> 
> Bugzilla States
> ===============
> In practice a bug sitting in these states matter less than the state
> of the fedora-review flag.  Participants are to follow these states
> as suggested guidelines, but the fedora-review flag has the hard 
> requirements of behavior.
> 
> NEW ASSIGNED REOPENED
> - There is no real distinction between these states.  The flag and 
> Assigned to pointer is what matters.
> - Note that ASSIGNED state is different from the Assigned pointer and 
> has no apparent relation for our purposes.
> 
> NEEDINFO
> - To owner or other person who needs to fix something or provide
> needed information in order to proceed further.
> 
> MODIFIED
> - Owner seems to have fixed it, but it requires testing.
> - OPTIONAL: you don't need to use this state.  It could sit in
> ASSIGNED where you do the same thing.
> - *Special Case: During the Mass Review, the fix may go into rawhide
> and the reviewer can verify both the CVS contents and package before
> giving fedora-review+.
> 
> CLOSED RAWHIDE
> - fedora-review+ is APPROVED, CVS procedure is done, and package is 
> built and confirmed to be done.
> - *Special Case*: During the Mass Review, it is fine to set to CLOSED 
> RAWHIDE if it is confirmed to be fixed there.  Please use MODIFIED
> prior to CLOSED RAWHIDE to allow for a verification step.
> 
> Review Process
> ==============
> 1. Review Request is filed
> 	fedora-review is BLANK
> 	Assigned to nobody
> 2. Reviewer Takes a Request
> 	fedora-review is ?
> 	Assigned to reviewer
> 3a. If review denied and needs work
> 	Comment
> 	fedora-review-
> 	NEEDINFO to whoever needs to fix it.

I see no value in flipping between - and ? on the fedora-review flag.
It doesn't provide any more information really. 
It will often not get done by the submitter since they don't know they
need to do so. 
It's another bugzilla knob to change on almost every exchange between
submitter and reviewer. 
If it has a 'DENIED' email like it does now for the core reviews, it
has a negative connotation and will make the submitter think they
aren't getting anywhere and should just give up. 

> 3b. fedora-review- and owner provides fix
> 	fedora-review back to ?, to request re-review
> 4. If APPROVED
> 	fedora-review+
> 	Assign to owner
> 5. After fedora-review+
> 	initiate the fedora-cvs request procedure
> 6. After fedora-cvs procedure
> 	checkin
> 	build
> 	verify buids
> 	set to CLOSED RAWHIDE
> 
> Other Possibilities
> ===================
> fedora-review? could also be used on any other Fedora bug when a
> horrible mess is found in an existing package, and attention for a
> re-review would be desired to fix it.  (Good idea, bad idea?)
> 
> Possible Process Optimizations
> ==============================
> 1. Changing fedora-review to ? auto-sets Assigned pointer to self.
> This is taking the review.
> 2. Changing fedora-review to + should auto-set Assigned pointer to 
> owner.  This is a little more difficult because it isn't always
> obvious who the owner is (especially in Mass Reviews), but this may
> be the reporter in regular reviews later.

Aside from the - flag usage I like this plan quite well... 

Hopefully some of the folks having problems with the current plans will
speak up and help it get fine tuned... 

kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-maintainers/attachments/20070207/de5f1e09/attachment.sig>