On 10/25/2011 11:07 PM, Hugh Brock wrote:
On Tue, Oct 25, 2011 at 04:13:37PM -0400, Mike Orazi wrote:
> On 10/25/2011 03:24 PM, Hugh Brock wrote:
>> On Tue, Oct 25, 2011 at 02:49:28PM -0400, Matt Wagner wrote:
>>> Hi all,
>>>
>>> I've been spending a little bit of time thinking about the past few
>>> days, when we frantically reviewed patches and fixed bugs in advance of
>>> today's demo. Since having a code freeze ahead of the actual deadline is
>>> something relatively new for us, I don't necessarily see it as a
>>> tremendous failing that we didn't nail the execution on the first try,
>>> but I've been thinking about the places I could do better next time. I
>>> share these musings here hoping to start a constructive discussion.
>>>
>
> Thanks for bringing this part up in particular. It seems to me that
> there was a fair bit of confusion around the code freeze date and
> not everyone was aware of it before the sprint started, which
> obviously made it hard to size the sprint to make it achievable.
>
> I'd like to propose that anything that needs review should be on the
> list by Wed, Nov 9 @ 5 PM EST. Then we cut a tag and a branch at a
> pre-defined time on Thursday Nov 10, say 3PM EST and anything that
> hasn't made it to master will need to explicitly cherry-picked into
> the branch for inclusion in the release. Any blocker bugs would be
> cherry-picked, but other patches can be discussed on an as-needed
> basis on list.
>
> The time after the branch is created will give us some amount of
> time to start digging into urgent bug fixes, do dry runs of the demo
> to, and to prepare more thoroughly for planning the next sprint.
> Also a fine time for 'out of band' activities that we tend to forget
> about like updated the Aeolus site.
>
>>> One thing is that I'd like to get into some sort of regular habit of
>>> reviewing patches on the mailing list. This sprint, I mostly thought,
>>> "I'm too busy writing code to bother with reviewing patches,"
which, in
>>> hindsight, was rather selfish to those who DID take time to review
>>> patches, and to those who needed patches approved. What I think I'd like
>>> to do is try to get into some sort of rhythm, where I maybe set aside 30
>>> minutes each day to review patches on the list. It occurs to me that an
>>> added bonus of reviewing patches is that I get a better sense for what
>>> other people are doing on the project.
>>>
>
> Yes! As things sit on the list, they potentially rot and honestly
> it is just harder to figure out what still needs to get onto master
> as release time rolls around.
>
>>> (As long as I'm publicly admitting my faults, I'd also like to stop
>>> using the morning scrum reports to blindly dump my list of tasks, and
>>> actually look at what others are working on. That, of course, is what
>>> I'm meant to be doing anyway, but it's easy to get in the habit of
just
>>> posting my status and moving on.)
>>>
>>> I'd also like to propose that we make a little more use of the Doctrine
>>> of Triviality. (I probably made that term up.) It seems like it's been
>>> acceptable for a long time to push trivial changes without sending them
>>> for review -- making a one-line change, reverting a broken commit, and
>>> so forth. While I am NOT proposing that developers push patches adding
>>> new features or making large changes on their own, there were a handful
>>> of patches on the list that were extremely trivial, or that were just
>>> obviously correct. Some of them sat for days waiting for someone to
>>> review them. I think it would be okay for people with commit access to
>>> push these things on their own, but note that they have a very high duty
>>> of care to make sure they're well-tested. How do other people feel about
>>> this concept? (For example -- I'd like to add some README files to the
>>> projects, just a quick overview mostly for the benefit of the GitHub
>>> pages. I sent one for Conductor some weeks ago that was ignored. I think
>>> it'd be fine to just push things like that.)
>>
>
> +1. I *thought* we reached a similar conclusion in the past and
> said it was just fine to push small changes. If there is violent
> disagreement the patch can always be reverted later.
>
>> Thanks so much for writing this up, Matt.
>>
>> I had an interesting conversation with a couple of folks this morning
>> along these lines. In fact, I'd like to take things a bit farther and
>> suggest that we could speed things up *a lot* if we removed the
>> requirement that each patch has to be applied and tested by someone
>> other than the author. It seems to me it's probably sufficient to ask
>> patch authors to test their own patches before posting them, and allow
>> reviewers to sign off after reading the code on-list. I think we could
>> *at least* leave this to the patch author's discretion -- if they
>> really want another pair of eyes on a new feature, great, but if
>> they're comfortable guaranteeing that the patch finishes a task
>> correctly and that all tests pass, an ACK based on reading the patch
>> should be sufficient.
>>
>> What do folks think of this?
>>
>
> Agree. It seems to me that most everyone has a fairly reasonable
> sense of when it would be a good idea to have a code/design review.
All right, I think this enough in favor that we should officially
change the ACK policy. From henceforth:
* We assume anyone who posts a patch to the list, has successfully run
an end-to-end test on it (or whatever is appropriate).
* Trivial and obviously correct patches may be pushed without an ACK,
but please do notify the list.
* Reviewers are not required to apply and e2e test patches, b/c it can
be assumed the author has already done so. If the patch is large, it
is in the author's interest to request review from an appropriate
community member.
* Authors who push patches that break tests will be slapped with a
fish appropriate to the size of the violation.
I know this is a bit after the fact, but I do want to voice my agreement
here.
I put it on our wiki so the points are not lost in the ephemera that is
aeolus-devel:
https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Patch_Reviews_...
There is one thing to point out here, though: all this applies to the
regular contributors.
When someone new (who doesn't have commit rights yet anyway) posts a
patch, I think we should still proceed with care, run the tests, etc.
Because there we can't readily assume the person sending the patch did
the due diligence.
Of course after a few good patches we will want have them
self-sufficient but it'd be good to apply more care and attention to the
newcomers.
Thomas
--Hugh
>
> Thanks again for firing this email off! I know you aren't the only
> one thinking about these things& really appreciate the discussion.
>
> Thanks,
> Mike