|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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?
Flags: needinfo?(ted.clancy) → needinfo?(ted)
The #build channel pointed at Mike as the source of truth. Mike, can you unblock me here?
Flags: needinfo?(ted) → needinfo?(mh+mozilla)
: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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Created attachment 8910713 [details] [diff] [review] bug1394891.diff 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.
Created attachment 8910809 [details] [diff] [review] bug1394891.diff 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.
Attachment #8910809 - Flags: review?(l10n) → review-
Attachment #8910809 - Attachment is obsolete: true
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.
Flags: needinfo?(gandalf) → needinfo?(dtownsend)
(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 > +  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
Attachment #8912672 - Flags: review?(l10n) → review-
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 :)
Attachment #8912672 - Flags: review?(l10n) → review?(glob)
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.
Attachment #8912672 - Flags: 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/#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 > +  the  here means the commit command is failing (because there's nothing to commit). you should remove this initial commit.
Attachment #8912672 - Flags: review?(glob) → review+
Pushed by firstname.lastname@example.org: 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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.