On 10/25/2011 12: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.
>
> 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.
>
> (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.)
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?
Take care,
--Hugh
This is a good proposal. Along these lines, I think the ack should
really be a code review between the author and someone who understands
the subsystem the code is written for. The author would give some
context on what the code is intended to do and explain how it was
tested. The reviewer would lend a second pair of eyes and a different
perspective to check if there was something the author may have
forgotten or if the code could be improved in certain ways.
it would be great if we can tighten also up our tests a bit. I think
we're missing a script that exercises both conductor and configure unit
tests and an additional piece that would do an end to end sanity check
for at least one provider. If we can automate the end-to-end a bit, it
would go a long way to making testing less burdensome, and if we pull it
into jenkins, a good way to do spot integration tests when new code is
checked in or when new aeolus components are pulled into the repo.
> 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
> myself.)
I think would should have a policy whereby people who are checking in
code to master should be monitoring the the builds to ensure they pass.
In the case where someone forgets to include a file in a commit, I would
say, if the author is online, the author should have a chance to fix the
failure. If the author is unavailable then I think, the code should be
reverted immediately.
> In any event, I do think we're making great progress, and
I'm really
> 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.
>
> Best,
> Matt
> _______________________________________________
> aeolus-devel mailing list
> aeolus-devel(a)lists.fedorahosted.org
>
https://fedorahosted.org/mailman/listinfo/aeolus-devel
>
>
Thanks for kicking off this discussion Matt.