> > Reverting the commit will often be cheaper to do than fixing the > regression in a follow-on patch, *but there'll undoubtedly be exceptions, > which we'll deal with (and learn from) as a team.* >
(Emphasis my own) As you've both pointed out, there are indeed instances where reverting a patch will be more expensive than simply fixing the regression in a follow-on patch. As with everything else in our profession, it's a judgement call, which require experience to make. I certainly wasn't trying to introduce "if it's broke, revert it" as a rule either. We should be aspiring to have master always ready to be deployed. Demanding that it's always deployable would be setting ourselves up for failure. Also is it a regression if the tests do not pick it up? Yes. A regression isn't a tree in a forest. We can't rely on tests to pick up regressions all the time. We don't have a dedicated QA team to defer to either. We're left with slowing down and being more deliberate in our functional review/sign off of patches. Taking a day a week to do a QA pass of the site also isn't out of the question. –Sam On Fri, Jun 12, 2015 at 8:36 PM, Jon Robson <[email protected]> wrote: > Sounds great Sam. I think the tricky thing is defining what a regression > means. For instance, if you are fixing a regression and introducing a less > important regression then what? Master is technically undeployable in both > states. > > Real world example: > Fixed to https://phabricator.wikimedia.org/T98498 caused > https://phabricator.wikimedia.org/T102215 > > Also is it a regression if the tests do not pick it up? > It seems that if we pick up a regression 2 weeks later, it's not almost > the less sensible/feasible to revert it but I'm not sure. > > Just stuff to think about. I agree in principle we need to be more > diligent and extreme when regressions happen. > > > On Fri, Jun 12, 2015 at 8:38 AM, Adam Baso <[email protected]> wrote: > >> Moving to mobile-l. >> >> On Friday, June 12, 2015, Sam Smith <[email protected]> wrote: >> >>> Hey web slingers, >>> >>> If there is a regression introduced by a patch, then please revert that >>> patch as soon as you've identified it and let the team know via >>> Phabricator, email, or both. Reverting the commit will often be cheaper to >>> do than fixing the regression in a follow-on patch, but there'll >>> undoubtedly be exceptions, which we'll deal with (and learn from) as a team. >>> >>> Fixing the regression in a follow-on patch means that: >>> >>> - *master won't be deployable* until the patch has been reviewed, >>> tested, and merged, which should be communicated to the Release >>> Engineering >>> team >>> - reviewers might have to drop what they're working on in order to >>> get it reviewed >>> - what if the original patch was lower priority? >>> - we should be cognisant of the cost of context switching >>> - the commit history will be dirty >>> >>> *Master should always be depoyable.* >>> >>> –Sam >>> >> >> _______________________________________________ >> Mobile-l mailing list >> [email protected] >> https://lists.wikimedia.org/mailman/listinfo/mobile-l >> >> > > > -- > Jon Robson > * http://jonrobson.me.uk > * https://www.facebook.com/jonrobson > * @rakugojon > > _______________________________________________ > Mobile-l mailing list > [email protected] > https://lists.wikimedia.org/mailman/listinfo/mobile-l > >
_______________________________________________ Mobile-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mobile-l
