All users were logged out of Bugzilla on October 13th, 2018

mozharness apk description script: Update only a single locale

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
For now, the script will update all locales.
We want to be able to update only a single (and specified) locale.
(Assignee)

Comment 1

4 years ago
Created attachment 8544701 [details] [diff] [review]
force-locale.diff

I also took the liberty to update the code to manage better urlopen failure with better error messages.
Let me know if you prefer a separate patch for this.
Attachment #8544701 - Flags: review?(bhearsum)
Comment on attachment 8544701 [details] [diff] [review]
force-locale.diff

Review of attachment 8544701 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/update_apk_description.py
@@ +52,5 @@
>          }],
> +        [["--force-locale"], {
> +            "dest": "force_locale",
> +            "help": "Force a specific locale (instead of all)",
> +            "default": "all"

A better idiom for this is to set a default, and check for existence. Eg:
if self.config.get("force_locale"):
  # use it
else:
  # get the full list


Is there ever going to be a need to specify multiple locales here? If so, it might be better to deal with that now - it shouldn't be too difficult.

@@ +119,5 @@
> +        if force_locale != "all":
> +            # The user wants to update only a single locale. Let him do that.
> +            # Return an array of one element
> +            return [force_locale]
> +        else:

This method's job is to get the full list of locales - I think it would be better to account for force_locale in update_desc. Eg, an "if self.config.get("force_locale")" block around https://github.com/mozilla/build-mozharness/blob/master/scripts/update_apk_description.py#L145.
Attachment #8544701 - Flags: review?(bhearsum) → feedback+
(Assignee)

Comment 3

4 years ago
Created attachment 8545274 [details] [diff] [review]
bug_1116745.diff

Right, much better this way!  Thanks for the feedback.

> Is there ever going to be a need to specify multiple locales here? 
I don't see a use case for this (yet?). This feature is for cases like bug 1111015 (Enable Ukrainian for Description Page on Google Play)
Attachment #8544701 - Attachment is obsolete: true
Attachment #8545274 - Flags: review?(bhearsum)
Attachment #8545274 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 4

4 years ago
Merged:
https://hg.mozilla.org/build/mozharness/rev/7fed494a6a8d

I also removed some useless print:
https://hg.mozilla.org/build/mozharness/rev/0ab6a26619f1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.