Closed Bug 1422487 Opened 7 years ago Closed 7 years ago

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


(Developer Services :: Mercurial:, defect)

Not set


(Not tracked)



(Reporter: aryx, Assigned: Pike)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.


Change with .ftl files:

Sheriffs tried to merge with central at but got the message that this needs review by one of the four people on the whitelist.
Flags: needinfo?(gps)
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.
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.
Summary: Fix the FTL whitelist hook to skip merges → Fix the FTL whitelist checks reviewers to pendically, should ignore case; and doesn't run on autoland
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?
glob can probably answer these questions, too, giving this more coverage. And/or do the review.
Flags: needinfo?(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.
Flags: needinfo?(glob)
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.
Component: Internationalization → Mercurial:
Product: Core → Developer Services
Attachment #8934145 - Attachment is obsolete: true
Attachment #8934145 - Flags: review?(gps)
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).

Comment on attachment 8933885 [details]
hooks: check reviewers case-independently (bug 1422487),
Attachment #8933885 - Flags: review?(gps) → review+
Pushed by
hooks: check reviewers case-independently , r=gps
Closed: 7 years ago
Resolution: --- → FIXED
This is deployed.
Flags: needinfo?(gps)
Blocks: 1423538
Blocks: 1423542
Blocks: 1423543
I've filed bugs 1423538, 1423542, 1423543 on the sub issues of autoland, commitparser, and test files, resp.
Assignee: nobody → l10n
You need to log in before you can comment on or make changes to this bug.