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
There are times when this works better but I disagree about it being a generally superior process. Patches are often not atomic; other patches can depend on them, sometimes without the author of either patch being aware. They might touch the same lines of code (in which case a revert would simply fail), or one patch can supply a function that the other patch calls (in which case the revert will break the site even worse, although a proper test suite should catch this), or they might include interacting CSS rules / HTML structure in which case the revert will introduce a new regression that can only be detected by regression testing. Regression testing reverts takes time - time which you could just spend on regression testing the fix. As for the commit history, reverts will make it dirty in a different way (one that confuses git bisect / pickaxe).
On Friday, June 12, 2015, Sam Smith samsmith@wikimedia.org wrote:
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.*
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 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
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: 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 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
-- Jon Robson
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l