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

[Jeremy C. Reed]

On Tue, 11 Dec 2012, Shane Kerr wrote:

> 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.

These are not my new proposals.  I was just pointing out that that the 
review guidelines do not match reality.

> > 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.

I do not want to do this.

> > 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.

I agree.

> > 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. 

I agree.

> > 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?

It does not break; just not all the tests are built and ran.  We can 
always force it set, but we do need to know the flags for it (and also 
we have two different flags due to incompatible gtest versions).

> > 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.

That is fine.

The point of my email was to fix the existing code review procedure. It 
is being considered for the best practices for use for other ISC 
development and it has the mistakes noted in my email. Okay if I fix it?