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 <jdlrobson@gmail.com> 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:

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 <abaso@wikimedia.org> wrote:
Moving to mobile-l. 

On Friday, June 12, 2015, Sam Smith <samsmith@wikimedia.org> 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
Mobile-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mobile-l




--

_______________________________________________
Mobile-l mailing list
Mobile-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mobile-l