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
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
> reviewing patches on the mailing list. This sprint, I mostly thought,
> "I'm too busy writing code to bother with reviewing patches," which,
> 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.
> As long as I'm proposing changes, I'd like to argue that we should
> consider test failures on master unacceptable, and revert any patch that
> introduces them. Checking in code with failing tests ends up causing
> huge headaches for other developers. For one, it prevents the demo
> server / Jenkins suite from running to completion. Second, it means
> that, for me to run tests on a patch, I need to run them twice -- once
> on the patch, and once on master, to make sure that the test failures I
> get aren't new failures in what I'm testing. What I'd propose is that,
> if code on master causes any test failures, we just revert the offending
> patch as soon as it's identified. (Of course, as with most of my other
> points, I don't mean to imply for a second that I'm not guilty of this
Yes. I'd like to suggest that we front-load the next sprint and say
that before any new features are committed we get back to clean test runs.
I would like to think that we can exercise some judgement on failures.
If it is fairly obvious and a 5 minute fix, go ahead and submit the
patch to fix things. For bigger, non-obvious errors rolling back
quickly sounds like a pretty reasonable plan to me.
> In any event, I do think we're making great progress, and
> glad to be a part of this team. But I figured that if I was thinking
> about these things, other people probably were, too, so I hope this is
> the start of a constructive discussion. Please let me know what you
> think, good or bad.
> aeolus-devel mailing list
Thanks again for firing this email off! I know you aren't the only one
thinking about these things & really appreciate the discussion.