Closed
Bug 1304323
Opened 8 years ago
Closed 8 years ago
Add the support of staged rollout in the push_apk script
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: Sylvestre)
Details
Attachments
(1 file)
The title says all
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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.
Attachment #8793250 -
Flags: review?(jlorenzo) → review+
Assignee | ||
Comment 3•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/29f122a5ade7 Add the support of staged rollout in the push_apk script r=jlorenzo
Assignee | ||
Comment 6•8 years ago
|
||
I just tried with 50.0b1 and it is working great now...
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29f122a5ade7
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce870ba0bb2
You need to log in
before you can comment on or make changes to this bug.
Description
•