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.)

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.) 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

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?

> 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

> 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.) > > 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 > > -- == 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 _______________________________________________ aeolus-devel mailing list aeolus-devel(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel

On Tue, Oct 25, 2011 at 03:38:45PM -0400, Chris Alfonso wrote: > Unfortunately, with the tests taking so long to run, with 30 minutes allocated you will probably only get through one patch. I know folks have looked into speeding up the specs, but to no avail. I think we will make a lot of progress and let the ack-ers spend more time developing if we rely on developers to ensure the tests pass on before they post, or after a rebase and repost.

Yes, amen! I can't say I've never mistakenly sent a broken patch, but I think you're spot-on here -- make sure all tests pass before you send your patch, rather than leaving it to the tester to discover. -- Matt _______________________________________________ aeolus-devel mailing list aeolus-devel(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel

I would propose we try to address this pain point be determining _why_ the tests are so slow. There is always more that can be done to speed them up, whether it be better use of mocks, changes to configuration, or something else. IMO, this should be a story in redmine, I added it here: https://www.aeolusproject.org/redmine/issues/2654

On Tue, Oct 25, 2011 at 03:24:31PM -0400, Hugh Brock wrote: > > 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. I think I initially misread this. To be clear, what you are suggesting is that developers would still send out a patch and wait for another contributor to give it an ACK, but the reviewer could give it an ACK without having to do end-to-end testing on it? (Or, if they were confident, potentially without doing any testing?) I think this is fine, if it's truly at the discretion of the reviewer. There are times I've spent 20 minutes reviewing a patch that really didn't need any sort of review. I'm just a little fearful that this is a slippery slope into _always_ just skimming patches and giving them an ACK without testing, even when they're involved patches that could break things. Overall I think this is a good thing, subject to the above caveat, though.

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.

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.

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 > 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.) >

> 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 > >

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

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.