1422487 - Fix the FTL whitelist checks reviewers to pendically, should ignore case; and doesn't run on autoland

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

No merges from autoland to central until the issue has been fixed or the hook disabled (again).

Merges with .ftl files still get rejected by the server after bug 1416366 got deployed earlier this week. Haven't check if this also applies to backouts.

Code: https://github.com/mozilla/version-control-tools/blob/7e7d2084fe3b358f76daceed7022324ecd02933b/hghooks/mozhghooks/check/prevent_ftl_changes.py#L56

Change with .ftl files: https://hg.mozilla.org/integration/autoland/rev/81cef8fbd704f73061c97f0940ef335d9242e1ac

Sheriffs tried to merge https://hg.mozilla.org/integration/autoland/rev/da64771ef8457997534d1b612388443ae3894c27 with central at https://hg.mozilla.org/mozilla-central/rev/0a559782a53a0d27424d3ec0c7f8a942ead597d7 but got the message that this needs review by one of the four people on the whitelist.

Comment 1

[Axel Hecht [:Pike]]

So, there are a couple of things.

The thing that busts the hook, is that this patch has an r=Pike, and pike is on the whitelist. I've got a test locally that validates that.

Which also means that the hook isn't running on autoland, which is concerning. gps?

And we should probably improve the test to exclude test files.

Comment 2

[Axel Hecht [:Pike]]

Attachment: hooks: check reviewers case-independently (bug 1422487),

Review commit: https://reviewboard.mozilla.org/r/204822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204822/

Comment 3

[Axel Hecht [:Pike]]

Not sure if the upper-lower-case part here should be more generic, or something we handle on a case-by-case basis.

But this has a test that failed with an r=Pike before the fix, and now passes.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you Axel for stepping in. I'm unfortunately not going to be in front of a keyboard till tomorrow.

Why is the parse_requal_reviewers case sensitive at all? It seems like it shouldn't be, not just for our use case?

Comment 5

[Axel Hecht [:Pike]]

glob can probably answer these questions, too, giving this more coverage. And/or do the review.

Comment 6

[:glob ✱]

i think zibi is right - `parse_reviewers` (which `parse_requal_reviewers` calls) should normalise returned reviewers to lowercase.  i'm surprised this hasn't bit us sooner.

Comment 7

[Axel Hecht [:Pike]]

Actually, for the reviewer and reviewers templates, we probably want to preserve casing? OTH, for the revsets, ignoring case might be a good feature.

I think it'd be good to take the existing patch as a bustage fix so that we can merge autoland again.

Also, still wondering why this wasn't kicking in on autoland to begin with.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8933885 [details]
hooks: check reviewers case-independently (bug 1422487),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204822/diff/1-2/

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

I think I know why the merge exception doesn't work.
Looking at the definition of https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/checks.py 
I think each changeset is considered in isolation, so the check passes for the merge changset, but then carries merrily on until the FTL-touching changeset and rejects that one, and thus the whole push.
Fortunately you can set state and then check it in post_check, so finding a merge or backout should set a flag, after which the check stops running.  I have a patch to do so.

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Attachment: hooks: Don't run FTL check on merges or backouts (bug 1422487),

Review commit: https://reviewboard.mozilla.org/r/205098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205098/

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

FYI this is holding up the WebRTC landing calendar [1]. We have patches scheduled to land all week, coordinated to land on different days for QA, and we're currently blocking on this from Friday (bug 1363667 comment 158 and bug 1388219 comment 73), and more landings due today (bug 1397793) and on (see link).

[1] https://docs.google.com/document/d/13tc3fTfoaE36fmThkAnKpwSFmkSff9wF63Ozuw7O25A/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8933885 [details]
hooks: check reviewers case-independently (bug 1422487),

https://reviewboard.mozilla.org/r/204822/#review210734

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/81e4d2af9520
hooks: check reviewers case-independently , r=gps

Comment 14

[Gregory Szorc [:gps]]

This is deployed.

Comment 15

[Axel Hecht [:Pike]]

I've filed bugs 1423538, 1423542, 1423543 on the sub issues of autoland, commitparser, and test files, resp.