Since the transition from DTD/properties to L20n is a massive undertake, we want to initially keep close control over which pieces are being localized using it. In order to be able to land the APIs but control how they're used, we'll want to introduce a whitelist that prevents new FTL files from being added to the build without :pike, :stas and :gandalf knowledge. This whitelist will be removed once we gain confidence in stability of the new API.

:ted, can you help me figure out the best location for this? Basically, we'd like to ensure that people don't start playing with FTL files without our knowledge. The easiest way I imagine this to work is to hook us into some build/packaging step and filter out all *.ftl files against some maintained whitelist. At the top of the whitelist we'd have a note saying to not add anything there without :pike's permission.

Maybe it'll work better if I ask the right Ted?

The #build channel pointed at Mike as the source of truth. Mike, can you unblock me here?

:gladium guided me on IRC. I'm going to write it as a hg hook based on https://hg.mozilla.org/hgcustom/version-control-tools/file/f254a80cbc80/hghooks/mozhghooks/prevent_string_changes.py It'll require r=pike, r=gandalf, r=flod or r=stas on any changes involving .ftl file.

Need to get the tests working to add more, but would be good to know if the patch is shaping up in line with Pike's vision.

Added tests! Ready for review :)

Comment on attachment 8910809 [details] [diff] [review] bug1394891.diff Review of attachment 8910809 [details] [diff] [review]: ----------------------------------------------------------------- There are a few problems in this patch. Also, I'd prefer to have this on mozreview in the next iteration. ::: hghooks/mozhghooks/check/prevent_ftl_changes.py @@ +47,5 @@ > + def name(self): > + return 'ftl_check' > + > + def relevant(self): > + return True This should do the same as the try config hook, return self.repo_metadata['firefox_releasing'] Also, should have tests to make sure that that happens. @@ +53,5 @@ > + def pre(self): > + pass > + > + def check(self, ctx): > + if any(f.endswith('.ftl') for f in ctx.files()): Should this be any ftl file, or just those exposed to localizers? Thinking about tests in particular.

Thanks Axel. Updated the patch to your review. > Should this be any ftl file, or just those exposed to localizers? Thinking about tests in particular. We talked with Axel about this and decided to keep the check applied to all .ftl files including tests.

Zibi, thinking about this patch I realized that we're changing tree rules, effectively. Do we have approval to do so? I suspect it'd be a good idea to have a stakeholder of tree rules to review this patch, too. (Not that I'd know who that is.)

Dave, can you advise here? Axel asked me to write this to keep a tight control over .ftl files in mozilla-central during the pilot phase in 58 and 59.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11) > Dave, can you advise here? Axel asked me to write this to keep a tight > control over .ftl files in mozilla-central during the pilot phase in 58 and > 59. As far as I'm concerned you folks own localisation and hence requiring your review for changes to pieces of localisation for a temporary period seems fine. That said I think you should post to dev-platform spelling out the plan so that everyone is aware of the reasons why and we can hear if there are any objections.

Great, thank you! Sent a post to dev.platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/Tij7yWeeXek

Sorry, I had this in mind a couple of days ago, and lost track. I wonder if we should should use the commitparser to find the reviewers, maybe parse_equal_reviewers, https://dxr.mozilla.org/hgcustom_version-control-tools/source/pylib/mozautomation/mozautomation/commitparser.py#140 But then again, I don't find any users of that ad-hoc, so maybe I'm confused. gps, what's the right API to use to find appropriate reviewers in new-style hooks?

Pike, it seems that :gps is offline till October 23rd. Is there anyone else we can NI instead?

yes, parse_requal_reviewers() is the best way to parse reviewers. > But then again, I don't find any users of that ad-hoc parse_requal_reviewers() was added after most of the hooks. they should be updated at some stage :)

Comment on attachment 8912672 [details] Bug 1394891 - Require an r+ from a list of reviewers to modify an FTL file. https://reviewboard.mozilla.org/r/184004/#review191596 Two issues that I found already, also, with gps afk, can you add glob as a second reviewer? ::: hghooks/mozhghooks/check/prevent_ftl_changes.py:58 (Diff revision 1) > + def pre(self): > + pass > + > + def check(self, ctx): > + if any(f.endswith('.ftl') for f in ctx.files()): > + if RE_PATTERN.search(ctx.description()): Per glob's comment, please use commitparser here. ::: hghooks/tests/test-prevent-ftl-changes.t:35 (Diff revision 1) > + ******************************************************** > + > + transaction abort! > + rollback completed > + abort: pretxnchangegroup.mozhooks hook failed > + [255] Looking at https://dxr.mozilla.org/hgcustom_version-control-tools/source/hghooks/tests/test-prevent-try-config.t the test should fail, AKA, the push should pass unless you touch server/.hg/IS_FIREFOX_REPO like in https://dxr.mozilla.org/hgcustom_version-control-tools/source/hghooks/tests/test-prevent-try-config.t#39

Excellent! Thank you Byron and Axel! I updated the patch to use commitparser (way cleaner!) and added the IS_FIREFOX_REPO (I have no idea why the test didn't fail before, but it fo sure did now without it). Re-requesting review :)

Comment on attachment 8912672 [details] Bug 1394891 - Require an r+ from a list of reviewers to modify an FTL file. https://reviewboard.mozilla.org/r/184004/#review193226 AFAICT, this looks good. Added glob as reviewer to make sure I didn't miss something.

Comment on attachment 8912672 [details] Bug 1394891 - Require an r+ from a list of reviewers to modify an FTL file. https://reviewboard.mozilla.org/r/184004/#review193432 lgtm – fix the nit in the test on commit. ::: hghooks/tests/test-prevent-ftl-changes.t:10 (Diff revision 2) > + $ hg -q commit -A -m initial > + [1] the [1] here means the commit command is failing (because there's nothing to commit). you should remove this initial commit.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/510a8c25bafe Require an r+ from a list of reviewers to modify an FTL file. r=pike,glob

This just got deployed as a ride-along with another deployment. If it causes problems, it is easy enough to disable (by commenting out references to prevent_ftl_changes in hghooks/mozhghooks/extensions.py in v-c-t.

The original announcement for this change said: "By All Hands we hope to be ready to remove the hook and enable everyone to use the new API. In the months to come, we'll be writing guidelines, tutorials, blog posts and other forms of prose[1] to get you all familiar with what changes and how to review patches for the new system. " It was posted just over a year ago, so 2 all-hands ago. When are we intending to turn off this commit hook?

Thanks for calling us out! The transition takes longer than expected, and at this moment I believe we should wait for two events: - Fluent enabled on startup path (bug 1441035) - Fluent 0.8 release (1.0 beta) We're tracking progress toward that in https://docs.google.com/spreadsheets/d/1ubpXFL9UMT9TgN76M3Jm8rGhDUFP6mLJrStbqJlY1M0/edit#gid=900263176 and we're in the open migration milestone right now. I'm sorry it takes longer, I'm doing my best to complete it.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #27) > The transition takes longer than expected, and at this moment I believe we > should wait for two events: > - Fluent enabled on startup path (bug 1441035) > - Fluent 0.8 release (1.0 beta) I would also like to see more tooling, for example to check FTL syntax sanity, before we remove the hook limitation.