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.
--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
--
== Hugh Brock, hbrock(a)redhat.com ==
== Engineering Manager, Cloud BU ==
== Aeolus Project: Manage virtual infrastructure across clouds. ==
==
http://aeolusproject.org ==
"I know that you believe you understand what you think I said, but I’m
not sure you realize that what you heard is not what I meant."
--Robert McCloskey