[bind10-dev] minor problems on the existing BIND10 review procedure

[Shane Kerr]

Jeremy,

Basically I agree with Michal: If it ain't broke, don't fix it. :)

I am of course in favor of modifying the review procedure if we expect
specific benefits, but I am not convinced with any of the proposals
here.

More below...

On Monday, 2012-12-10 12:30:26 -0600, 
"Jeremy C. Reed" <jreed at isc.org> wrote:
> Reviewing http://bind10.isc.org/wiki/CodeReviewProcedure
> as part of ISC Engineering Best Practices board work.
> (bottom says Review scheduled 2010-12-18)
> 
> Tickets are not created specifically for a "review".

This is true. Neither for BIND 9 nor BIND 10 do we create tickets
specifically for review.

It seems like extra work, where we would have to create an extra ticket
and link it to the one where the work was done. Plus it seems like it
would lead to confusion, since casual users of the ticket system would
see apparently duplicate tickets with different status and owners.

If there is a proposal to change this, I'd like to understand the
motivation.

> Ticket titles do not say "review" in them.

This is also true. Again, we do this neither for BIND 9 nor BIND 10.

In the case of BIND 9, we have a separate queue for review, so it is
clear which tickets are being reviewed.

In the case of BIND 10, review tickets have the status "review", so
again it is quite clear.

Unless we plan on creating extra tickets (as proposed above), then we'd
need to rename tickets to meet this, which seems like extra work for no
gain.

> The method of indicating (not "announced") is setting the ticket to 
> review state that is unowned. No announcement is sent to list nor to 
> manager.

Yes, this is correct. I'm not sure what purpose having an announcement
would serve, other than to add another 5 lines to my .procmailrc file.
Both the BIND 9 and BIND 10 teams are excellent at performing timely
reviews, because they look at the state of the ticket queues regularly
in the course of normal work. 

> make distcheck is only correct to run tests if --with-gtest is used 
> (also maybe misleading since this doesn't do the system level checks).

Hm... okay this is interesting. Does "make distcheck" break if
--with-gtest is not used? It should. If not, is there a straightforward
way to change this?

> Usually the checklist is not copy-and-pasted in response(s).

This is true. However since I think the BIND 10 developers probably do
an average of 3 reviews a week each I'm not sure if this is necessary.

--
Shane