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

Categories

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

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aryx, Assigned: Pike)

References

(Blocks 1 open bug)

Details

Attachments

(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. 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.
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: hg.mozilla.org
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). [1] https://docs.google.com/document/d/13tc3fTfoaE36fmThkAnKpwSFmkSff9wF63Ozuw7O25A/
Attachment #8933885 - Flags: review?(gps) → review+
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/81e4d2af9520 hooks: check reviewers case-independently , r=gps
Status: NEW → RESOLVED
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.

Attachment

General

Created:
Updated:
Size: