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

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

2 years ago
Blocks: 1365426
Priority: -- → P3
: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)
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.
Flags: needinfo?(mh+mozilla)
Assignee

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Posted patch bug1394891.diff (obsolete) — Splinter Review
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)
Posted patch bug1394891.diff (obsolete) — Splinter Review
Added tests! Ready for review :)
Attachment #8910713 - Attachment is obsolete: true
Attachment #8910713 - Flags: feedback?(l10n)
Attachment #8910809 - Flags: review?(l10n)
Assignee

Updated

2 years ago
Blocks: 1402061
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-
Assignee

Updated

2 years ago
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.)
Flags: needinfo?(gandalf)
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)
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)
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

2 years 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)
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

2 years ago
Attachment #8912672 - Flags: review?(l10n) → review?(glob)

Comment 20

2 years 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

2 years 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

2 years 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: 2 years 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.

Comment 26

7 months ago
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?
Flags: needinfo?(gandalf)
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.
Flags: needinfo?(gandalf)
(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.
You need to log in before you can comment on or make changes to this bug.