Auto-rebaseline CLs are landed directly |
|||
Issue descriptionAccording to wangxianzhu@, the script "webkit-patch rebaseline-cl" creates a CL and automatically lands it, with: TBR=<someone> NOTRY=true NOPRESUBMIT=true For example, [1]. This goes against explicit advice on chromium-dev [2] that we never use NOTRY and NOPRESUBMIT except in extreme circumstances (i.e., a sheriff who needs to push something through a broken CQ, etc -- even sheriffs should normally not use NOTRY and let their disable CLs go through the normal CQ). There is generally no urgency with rebaseline CLs and they do not deserve the special treatment of rushing them through with TBR, NOTRY and NOPRESUBMIT. [1] https://codereview.chromium.org/2694623015 [2] https://groups.google.com/a/chromium.org/forum/#!topic/infra-dev/lCrUoPTVbMo
,
Feb 17 2017
This is my comment in the CL: > This is done with a script "webkit-patch rebaseline-cl", just like an manual > version of auto-rebaseline on the rebaseline bot which automatically lands > auto-rebaselines with NOTRY and NOPRESUBMIT. It contains typos (though corrected in a later comment), inaccuracies, and is confusing. I apologize. I rewrite it: This CL was created by 'webkit-patch rebaseline-expectations', just like a manual version of auto-rebaseline on the rebaseline bot which automatically lands auto-rebaseline CLs directly using 'git cl land' (which is basically equivalent to NOTRY/NOPRESUBMIT plus 'git cl set-commit). I think it's intentional that auto-rebaseline bot lands auto-rebaseline CLs directly, because rebaselines are supposed to always succeed in theory. If they fail for some reason, we just need to rebaseline again. An alternate way of the rebaseline is to schedule an auto-rebaseline, but I think that wastes time waiting for the waterfall bots to all pass the CL which scheduled the auto-rebaseline, because all actual results have been ready on the waterfall bots after they failed.
,
Feb 17 2017
#2 I will ask around but I think using "git cl land", NOTRY, or any other way to skip the CQ is just asking for trouble. We use to do this sort of thing waaaaay back in the day when we were "pretty sure" or when the CQ was taking too long, but it's long been discouraged. No matter how sure you are (and whether you're a human or a script), you can make mistakes and we want the CQ there to catch those mistakes BEFORE they end up going in the waterfall and making sheriffs diagnose the problem.
,
Feb 17 2017
I agree with you about the risks. However, CQ didn't catch the original failure, because we don't have full coverage of all OS variants in CQ. Even if I let the rebaseline go through CQ, there might be still chance of failure on waterfall bots. Sometimes when I was not sure if a CL would break waterfall bot after passing CQ, I thought of adding something like "To sheriff: if this CL breaks any layout test on Mac10.11, Win7, etc, please don't revert but mark it failure and file a bug to me" in the CL description, but never actually did it. The more appropriate way should be to land (directly) a CL adding NeedsRebaseline and let the auto-rebaseline bot land the rebaseline, instead of rebaselining by myself.
,
Aug 4 2017
Rebaseline-o-matic was turned down, so this is no longer an issue. |
|||
►
Sign in to add a comment |
|||
Comment 1 by mgiuca@chromium.org
, Feb 17 2017