1304323 - Add the support of staged rollout in the push_apk script

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

The title says all

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1304323 - Add the support of staged rollout in the push_apk script

Review commit: https://reviewboard.mozilla.org/r/80042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80042/

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8793250 [details]
Bug 1304323 - Add the support of staged rollout in the push_apk script

https://reviewboard.mozilla.org/r/80042/#review78746

LGTM modulo a few nits.

::: testing/mozharness/scripts/push_apk.py:68
(Diff revision 1)
>              "help": "The path to the ARM v7 API v15 APK file",
>          }],
> +        [["--rollout-percentage"], {
> +            "dest": "rollout_percentage",
> +            "help": "The rollout percentage (update percentage)",
> +            "default": "None"

Nit: How about `None` instead of `"None"`?

::: testing/mozharness/scripts/push_apk.py:132
(Diff revision 1)
> +        if self.config['track'] == "rollout" and self.config["rollout_percentage"] is "None":
> +            self.fatal("When using track='rollout', --rollout-percentage must be provided too")
> +
> +        if self.config["rollout_percentage"] is not "None":
> +            self.percentage = float(self.config["rollout_percentage"])
> +            if self.percentage < 0 or self.percentage >= 1:

Nit: Setting the value to 1 will be reported as an error, but the error message doesn't explain why.

::: testing/mozharness/scripts/push_apk.py:133
(Diff revision 1)
> +            self.fatal("When using track='rollout', --rollout-percentage must be provided too")
> +
> +        if self.config["rollout_percentage"] is not "None":
> +            self.percentage = float(self.config["rollout_percentage"])
> +            if self.percentage < 0 or self.percentage >= 1:
> +                self.fatal("Percentage should be between 0 and 1")

I believe the word "percentage" is odd when being used with 0 to 1 values. 

I'd either rename it to "rate", or allow values between 0 and 100. I have a preference for the second one, because "rate" is ambiguous and you can't really guess what's the valid range without actively reading the --help message.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8793250 [details]
Bug 1304323 - Add the support of staged rollout in the push_apk script

https://reviewboard.mozilla.org/r/80042/#review78746

> Nit: How about `None` instead of `"None"`?

Doesn't work unfortunatelly :/

> I believe the word "percentage" is odd when being used with 0 to 1 values. 
> 
> I'd either rename it to "rate", or allow values between 0 and 100. I have a preference for the second one, because "rate" is ambiguous and you can't really guess what's the valid range without actively reading the --help message.

*100 :)

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8793250 [details]
Bug 1304323 - Add the support of staged rollout in the push_apk script

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80042/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29f122a5ade7
Add the support of staged rollout in the push_apk script r=jlorenzo

Comment 6

[Sylvestre Ledru [:Sylvestre]]

I just tried with 50.0b1 and it is working great now...

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29f122a5ade7

Comment 8

[Sylvestre Ledru [:Sylvestre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce870ba0bb2