FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > CentOS > CentOS

 
 
LinkBack Thread Tools
 
Old 10-23-2008, 08:40 AM
Hans de Goede
 
Default Adding reviewing to our development process

Hi All,

As I've been slowly getting into anaconda development I've become increasingly
amazed with the number of regressions we have caused by typos and other small
predictable errors in day to day development work.


So I've been thinking that we really need to change our processes to include a
review phase for all patches except the most trivial ones (the ones fixing all
the typos).


This blog post was the trigger for me to stop just thinking about this and
actually write a mail about it:

http://anholt.livejournal.com/39921.html

I would like to invite you all to read this post, esp. those who are going to
say no against my proposal to start doing more reviews as part of anaconda
development.


Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-23-2008, 01:04 PM
Jeremy Katz
 
Default Adding reviewing to our development process

On Thu, 2008-10-23 at 10:40 +0200, Hans de Goede wrote:
> So I've been thinking that we really need to change our processes to include a
> review phase for all patches except the most trivial ones (the ones fixing all
> the typos).
>
> This blog post was the trigger for me to stop just thinking about this and
> actually write a mail about it:
> http://anholt.livejournal.com/39921.html
>
> I would like to invite you all to read this post, esp. those who are going to
> say no against my proposal to start doing more reviews as part of anaconda
> development.

It's something I've thought about from time to time as well. We've
gotten better with sending larger patches to the list for review (though
there's still room for some improvement) which was really sort of a
first step.

The biggest "problem" with doing so is that often changes need to be
done "now" so that a snapshot or a beta, etc can be built. And with the
geographical dispersion of people, that will frequently lead to
bottlenecks and the unfortunate side effect of blocking a release
candidate.

But that said, the goal would be that by having such pre-commit reviews,
we could hopefully reduce the number of times that we're blocking a
release tree. So I definitely think it's worth trying. The question
comes down to when do we start doing it:
1) Now
2) At the beginning of the F11 development cycle

Given the risk for slowing down a release that really has no room to be
slipped, I lean a little bit towards the latter. But opinions to the
contrary could easily sway me otherwise. Especially if Jesse was
on-board.

The other thing that we could do is redirect the commits to be sent
here; but that just gets post-commit review which (in theory) we already
have at least some of. And at that point, it's too late if there's been
a build...

Jeremy

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-23-2008, 01:40 PM
Hans de Goede
 
Default Adding reviewing to our development process

Hi all,

Jeremy Katz wrote:

On Thu, 2008-10-23 at 10:40 +0200, Hans de Goede wrote:
So I've been thinking that we really need to change our processes to include a
review phase for all patches except the most trivial ones (the ones fixing all
the typos).




<snip>



It's something I've thought about from time to time as well. We've
gotten better with sending larger patches to the list for review (though
there's still room for some improvement) which was really sort of a
first step.



<snip>



But that said, the goal would be that by having such pre-commit reviews,
we could hopefully reduce the number of times that we're blocking a
release tree. So I definitely think it's worth trying. The question
comes down to when do we start doing it:
1) Now
2) At the beginning of the F11 development cycle

Given the risk for slowing down a release that really has no room to be
slipped, I lean a little bit towards the latter. But opinions to the
contrary could easily sway me otherwise. Especially if Jesse was
on-board.



I agree lets start doing this for F-11, I think we also need to write something
up how the process will look (something like 10-20 lines of text, no prose
please) does anyone have a suggestion how to shape this?


Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-23-2008, 01:51 PM
Chris Lumens
 
Default Adding reviewing to our development process

> As I've been slowly getting into anaconda development I've become
> increasingly amazed with the number of regressions we have caused by
> typos and other small predictable errors in day to day development work.
>
> So I've been thinking that we really need to change our processes to
> include a review phase for all patches except the most trivial ones (the
> ones fixing all the typos).

Two things:

(1) We already have the commit mails, which we theoretically could all
be reading and finding problems in. Yet, I don't see a lot of followup
comments in IRC regarding typos. Jeremy has some from time to time, and
I do too but very rarely. I wonder how effective it's really going to
be.

(2) If we go down this road, we need to make sure it's not mandatory.
That is, I still need to be able to commit emergency things to do a
build right now without having to wait for other people to comment.
That's just the nature of anaconda development from time to time.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-23-2008, 02:24 PM
Jeremy Katz
 
Default Adding reviewing to our development process

On Thu, 2008-10-23 at 15:40 +0200, Hans de Goede wrote:
> I agree lets start doing this for F-11, I think we also need to write something
> up how the process will look (something like 10-20 lines of text, no prose
> please) does anyone have a suggestion how to shape this?

I'll try to write up something basic in one of the breaks between
sessions of the conference I'm at today

Jeremy

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-24-2008, 12:25 PM
John Summerfield
 
Default Adding reviewing to our development process

Jeremy Katz wrote:

On Thu, 2008-10-23 at 15:40 +0200, Hans de Goede wrote:
I agree lets start doing this for F-11, I think we also need to write something
up how the process will look (something like 10-20 lines of text, no prose
please) does anyone have a suggestion how to shape this?


I'll try to write up something basic in one of the breaks between
sessions of the conference I'm at today


If there be a patch queue that can be used for "urgent" rebuilds, that
the review team has to okay before firm committal, that might address
your concern Jeremy, while ensuring that they don't fall through the cracks.


In support of the idea, let may say that there are many errors in
writing a program.


Errors that the compiler picks up cause little trouble and little cost,
Ordinarily, they will be picket up and fixed in a run or two.


Errors the coder picks up, mostly those where the program always
segfaults, produces incorrect output also cause little trouble, mostly
they are picked up in a day or two and involve one person.


From here, they get more expensive:-(

If the team does code reviews (an idea that's been around 20 and more
years I know of), and finds errors, those errors have already involved a
few people and taken longer to fix.


However, errors can slip past, and if there's a QA team testing the
software, then there is an opportunity to do exhaustive (and expensive)
testing. Everyone developing important software should to this, though
my experience with websites (including big stockbrokers and banks)
suggests they don't.


Errors getting past QA (and I will include "Alpha" and "Beta" testing as
later phases of QA) get shipped to customers.


From this point they get serious because they interfere with customers'
work, maybe seriously, and the process of getting them fixed is very
expensive: they may only occur is peculiar circumstances, maybe with
obscure hardware or timing, there may be lots of going back and forth,
and in extreme circumstances lawyers and courts may be involved.


And, ordinarily, fixes have to go through all the above processes before
being ready for distribution.


Bear in mind too that bugs might not be coding errors, they can be
problems in documentation, or even design (specification) problems:
maybe the designers' understanding of the problem was flawed.


If all patches are reviewed before being committed, that will improve
the quality of the code that ships to the next stage. How much depends
on the effort applied in the review process, and in any event is
impossible to gauge. It might be interesting though to record how many
are found, and maybe occasionally review problems found (and not found
until later) to see whether procedures can be improved further.



.
--

Cheers
John

-- spambait
1aaaaaaa@coco.merseine.nu Z1aaaaaaa@coco.merseine.nu
-- Advice
http://webfoot.com/advice/email.top.php
http://www.catb.org/~esr/faqs/smart-questions.html
http://support.microsoft.com/kb/555375

You cannot reply off-list:-)

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-24-2008, 12:34 PM
John Summerfield
 
Default Adding reviewing to our development process

Chris Lumens wrote:
As I've been slowly getting into anaconda development I've become
increasingly amazed with the number of regressions we have caused by
typos and other small predictable errors in day to day development work.


So I've been thinking that we really need to change our processes to
include a review phase for all patches except the most trivial ones (the
ones fixing all the typos).


Two things:

(1) We already have the commit mails, which we theoretically could all
be reading and finding problems in. Yet, I don't see a lot of followup
comments in IRC regarding typos. Jeremy has some from time to time, and
I do too but very rarely. I wonder how effective it's really going to
be.


It has to be compulsory.


(2) If we go down this road, we need to make sure it's not mandatory.
That is, I still need to be able to commit emergency things to do a
build right now without having to wait for other people to comment.
That's just the nature of anaconda development from time to time.


Every software project needs a process in place for emergency fixes, but
they cannot be applied to the main code base until approved.


In the early 80s, I worked for SPL (Australia) Pty Limited, and I was
flown from Canberra to Melbourne to fix a problem with software we
supplied to Dulux Australia (a paint manufacturer).


I found the problem and a fix (and they were very happy)[1], but the fix
was not the fix that to software supplier (Software AG) incorporated
into the code.



[1] The Boss was happy too.


Don't look to avoid the code review, just accept that it's necessary and
find a way to work with it. Maybe a branch of the project for each occasion?





--

Cheers
John

-- spambait
1aaaaaaa@coco.merseine.nu Z1aaaaaaa@coco.merseine.nu
-- Advice
http://webfoot.com/advice/email.top.php
http://www.catb.org/~esr/faqs/smart-questions.html
http://support.microsoft.com/kb/555375

You cannot reply off-list:-)

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-24-2008, 10:38 PM
David Cantrell
 
Default Adding reviewing to our development process

On Thu, Oct 23, 2008 at 09:51:46AM -0400, Chris Lumens wrote:
> > As I've been slowly getting into anaconda development I've become
> > increasingly amazed with the number of regressions we have caused by
> > typos and other small predictable errors in day to day development work.
> >
> > So I've been thinking that we really need to change our processes to
> > include a review phase for all patches except the most trivial ones (the
> > ones fixing all the typos).
>
> Two things:
>
> (1) We already have the commit mails, which we theoretically could all
> be reading and finding problems in. Yet, I don't see a lot of followup
> comments in IRC regarding typos. Jeremy has some from time to time, and
> I do too but very rarely. I wonder how effective it's really going to
> be.
>
> (2) If we go down this road, we need to make sure it's not mandatory.
> That is, I still need to be able to commit emergency things to do a
> build right now without having to wait for other people to comment.
> That's just the nature of anaconda development from time to time.

We do have the commit mails and most of us read them, but sometimes not. It's
not really guaranteed that someone will review the code. Plus, it could be
too late once someone reads it since a build could have already gone through.

What about a process like this:

* Create new review branches for the active development trees. One for
rawhide, one for rhel4-branch, one for rhel5-branch. Whatever we need,
really:

master-review
rhel5-branch-review
rhel4-branch-review

Or some other names (rawrawhide, for example).

* If you want code reviewed by someone else before it goes in to the mainline,
commit it to the review branch for the release you're working on. A commit
mail will go out to everyone and we will also see code come in if we are
keeping local copies of the review branches (which we all should anyway).

* Establish review rules. Such as, you can't review your own code. Someone
else on the team reviews the patch on the review branch and if they like
it, cherry-pick it to the mainline and use the Signed-Off By feature of
git. That way we get two people to blame if a patch goes bad.

By using review branches to stage code, we can all continue to work at our on
pace and still get reviews from other people. We can at least ensure that
what gets built from rawhide is more stable...or at least looked at by two
people.

Using review branches also keeps the door open to emergency fixes that need to
go and we don't really have to change our processes for that.

Thoughts?

--
David Cantrell <dcantrell@redhat.com>
Red Hat / Honolulu, HI
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-24-2008, 10:42 PM
David Cantrell
 
Default Adding reviewing to our development process

On Fri, Oct 24, 2008 at 12:38:59PM -1000, David Cantrell wrote:
> On Thu, Oct 23, 2008 at 09:51:46AM -0400, Chris Lumens wrote:
> > > As I've been slowly getting into anaconda development I've become
> > > increasingly amazed with the number of regressions we have caused by
> > > typos and other small predictable errors in day to day development work.
> > >
> > > So I've been thinking that we really need to change our processes to
> > > include a review phase for all patches except the most trivial ones (the
> > > ones fixing all the typos).
> >
> > Two things:
> >
> > (1) We already have the commit mails, which we theoretically could all
> > be reading and finding problems in. Yet, I don't see a lot of followup
> > comments in IRC regarding typos. Jeremy has some from time to time, and
> > I do too but very rarely. I wonder how effective it's really going to
> > be.
> >
> > (2) If we go down this road, we need to make sure it's not mandatory.
> > That is, I still need to be able to commit emergency things to do a
> > build right now without having to wait for other people to comment.
> > That's just the nature of anaconda development from time to time.
>
> We do have the commit mails and most of us read them, but sometimes not. It's
> not really guaranteed that someone will review the code. Plus, it could be
> too late once someone reads it since a build could have already gone through.
>
> What about a process like this:
>
> * Create new review branches for the active development trees. One for
> rawhide, one for rhel4-branch, one for rhel5-branch. Whatever we need,
> really:
>
> master-review
> rhel5-branch-review
> rhel4-branch-review
>
> Or some other names (rawrawhide, for example).
>
> * If you want code reviewed by someone else before it goes in to the mainline,
> commit it to the review branch for the release you're working on. A commit
> mail will go out to everyone and we will also see code come in if we are
> keeping local copies of the review branches (which we all should anyway).
>
> * Establish review rules. Such as, you can't review your own code. Someone
> else on the team reviews the patch on the review branch and if they like
> it, cherry-pick it to the mainline and use the Signed-Off By feature of
> git. That way we get two people to blame if a patch goes bad.
>
> By using review branches to stage code, we can all continue to work at our on
> pace and still get reviews from other people. We can at least ensure that
> what gets built from rawhide is more stable...or at least looked at by two
> people.
>
> Using review branches also keeps the door open to emergency fixes that need to
> go and we don't really have to change our processes for that.
>
> Thoughts?

Also forgot to mention that I think a review branch system like this would
work well for us since we are geographically spread out. The likelihood of me
reviewing code from Hans or code from Brno would be just as likely as me
reviewing code from Westford or Huntsville. At least on paper, not sure how
it would work out in practice. I guess it depends on how much we all take on
for a given release.

--
David Cantrell <dcantrell@redhat.com>
Red Hat / Honolulu, HI
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 10-25-2008, 08:17 AM
Hans de Goede
 
Default Adding reviewing to our development process

David Cantrell wrote:

On Thu, Oct 23, 2008 at 09:51:46AM -0400, Chris Lumens wrote:
As I've been slowly getting into anaconda development I've become
increasingly amazed with the number of regressions we have caused by
typos and other small predictable errors in day to day development work.


So I've been thinking that we really need to change our processes to
include a review phase for all patches except the most trivial ones (the
ones fixing all the typos).

Two things:

(1) We already have the commit mails, which we theoretically could all
be reading and finding problems in. Yet, I don't see a lot of followup
comments in IRC regarding typos. Jeremy has some from time to time, and
I do too but very rarely. I wonder how effective it's really going to
be.

(2) If we go down this road, we need to make sure it's not mandatory.
That is, I still need to be able to commit emergency things to do a
build right now without having to wait for other people to comment.
That's just the nature of anaconda development from time to time.


We do have the commit mails and most of us read them, but sometimes not. It's
not really guaranteed that someone will review the code. Plus, it could be
too late once someone reads it since a build could have already gone through.

What about a process like this:

* Create new review branches for the active development trees. One for
rawhide, one for rhel4-branch, one for rhel5-branch. Whatever we need,
really:

master-review
rhel5-branch-review
rhel4-branch-review

Or some other names (rawrawhide, for example).

* If you want code reviewed by someone else before it goes in to the mainline,
commit it to the review branch for the release you're working on. A commit
mail will go out to everyone and we will also see code come in if we are
keeping local copies of the review branches (which we all should anyway).

* Establish review rules. Such as, you can't review your own code. Someone
else on the team reviews the patch on the review branch and if they like
it, cherry-pick it to the mainline and use the Signed-Off By feature of
git. That way we get two people to blame if a patch goes bad.

By using review branches to stage code, we can all continue to work at our on
pace and still get reviews from other people. We can at least ensure that
what gets built from rawhide is more stable...or at least looked at by two
people.

Using review branches also keeps the door open to emergency fixes that need to
go and we don't really have to change our processes for that.

Thoughts?



David,

Thanks for your input I think this could work, I say I think because I'm not
very familiar with git.


Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 

Thread Tools




All times are GMT. The time now is 04:50 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org