On 14 December 2015 at 15:39, Katherine Cox-Buday <katherine.cox-bu...@canonical.com> wrote: > Hey All, > > I think we have a mis-alignment in how we currently do reviews and feature > branches. > > We've switched over to feature-branches which is great and has allowed > Moonstone to land "good enough" code into our feature branch to support a > bi-weekly demo and solicit feedback. At the same time, I feel like we're > wasting people's time asking for a +1 on code that is not intended to be > landed into master. Often this code will take shortcuts or stub out some > code, and as the lead I'll make a judgment call to circle-back later. > Reviewers don't necessarily know this. > > Conversely, when we go to land the feature branch into master, these PRs are > generally rubber-stamped. > > I feel like maybe we have this backwards?
I feel your pain. But the other side of this coin is that feature branches can be very big, so it's very difficult to give them a comprehensive review when merging them into master. For example, this feature branch (https://github.com/juju/juju/pull/3590) involved more than 16000 lines of changes ("diff" prints 30616 lines). I wouldn't have any trust in the results of a code review of all that code at once. I think that if you're going to implement features that you don't want comprehensively reviewed, it's best to treat them as "spikes" and keep them out of the juju repo entirely, then try to land them in manageable chunks on the feature branch (or master). In my view, an important attribute of the feature branches is that they are solidly reviewed and thus OK to land without full review of the entire diff. That said, I believe that it is *also* important that people do try to review feature branches when they merge, if at all reasonable. It's often the first time that people not directly involved with the feature branch actually look at the implications for master. cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
