Add a temporary whitelist of files localized via L20n to control the exposure

RESOLVED FIXED

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 months ago
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.
(Assignee)

Updated

6 months ago
Blocks: 1365426
Priority: -- → P3
(Assignee)

Comment 1

6 months ago
: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.
Flags: needinfo?(ted.clancy)
(Assignee)

Comment 2

5 months ago
Maybe it'll work better if I ask the right Ted?
Flags: needinfo?(ted.clancy) → needinfo?(ted)
(Assignee)

Comment 3

5 months ago
The #build channel pointed at Mike as the source of truth.

Mike, can you unblock me here?
Flags: needinfo?(ted) → needinfo?(mh+mozilla)
(Assignee)

Comment 4

5 months ago
: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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

5 months ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 months ago
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.
Attachment #8910713 - Flags: feedback?(l10n)
(Assignee)

Comment 6

5 months ago
Created attachment 8910809 [details] [diff] [review]
bug1394891.diff

Added tests! Ready for review :)
Attachment #8910713 - Attachment is obsolete: true
Attachment #8910713 - Flags: feedback?(l10n)
Attachment #8910809 - Flags: review?(l10n)
(Assignee)

Updated

5 months ago
Blocks: 1402061

Comment 7

5 months ago
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-
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8910809 - Attachment is obsolete: true
(Assignee)

Comment 9

5 months ago
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.

Comment 10

5 months ago
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.)
Flags: needinfo?(gandalf)
(Assignee)

Comment 11

5 months ago
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.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 13

5 months ago
Great, thank you!

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

Comment 14

5 months ago
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?
Flags: needinfo?(gps)
(Assignee)

Comment 15

4 months ago
Pike, it seems that :gps is offline till October 23rd.

Is there anyone else we can NI instead?
Flags: needinfo?(l10n)
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 :)
Flags: needinfo?(l10n)
Flags: needinfo?(gps)

Comment 17

4 months ago
mozreview-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/#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
Attachment #8912672 - Flags: review?(l10n) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 19

4 months ago
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 :)

Updated

4 months ago
Attachment #8912672 - Flags: review?(l10n) → review?(glob)

Comment 20

4 months ago
mozreview-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.
Attachment #8912672 - Flags: review+

Comment 21

4 months ago
mozreview-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
> +  [1]

the [1] 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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

4 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED

Comment 25

4 months ago
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.