-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Updates Fenix taskcluster tasks to support beta release #1893
Updates Fenix taskcluster tasks to support beta release #1893
Conversation
4400e20
to
f4e79d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. I like the way beta is supported! I found some discrepancies between .taskcluster.yml
and decision.py
. No big deal as they would be caught by a staging release/nightly.
e8708d9
to
3d27522
Compare
3d27522
to
d49c798
Compare
metadata: | ||
name: Fenix - Decision task | ||
description: Schedules the build and test tasks for Fenix. | ||
- $if: 'tasks_for == "github-release"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that right now, any github release is assumed to be beta
.
As we get closer to the Fenix Preview release, I'll add a parse check to see if the tag has -beta.*
on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ \o/
Let's fix the colliding Treeherder symbols before landing, though.
.taskcluster.yml
Outdated
&& python automation/taskcluster/decision_task.py beta ${event.release.tag_name} | ||
extra: | ||
treeherder: | ||
symbol: B |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: B
might be confused with "build", for people who come from mozilla-central. We might want to go with beta
. I'm fine changing N
, for the sake of consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I'm fine changing N, for the sake of consistency.
As in changing N
to nightly
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right :)
@@ -25,8 +27,8 @@ android { | |||
applicationId "org.mozilla.fenix" | |||
minSdkVersion Config.minSdkVersion | |||
targetSdkVersion Config.targetSdkVersion | |||
versionCode Config.versionCode | |||
versionName Config.versionName + Config.generateVersionSuffix() | |||
versionCode 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: do we still want to keep versionCode 1
. My understanding was that very first APK has already been uploaded to Google Play.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that the first APK has already been uploaded to Google Play.
Config.versionCode
used to just be 1
, so I'm just inlining that value to the one place it's used (here) so it's more obvious at-a-glance.
Note that this is overridden separately here for release builds :)
@@ -387,4 +409,4 @@ task printGeckoviewVersions { | |||
|
|||
// Normally this should use the same version as the glean dependency. But since we are currently using AC snapshots we | |||
// can't reference a git tag with a specific version here. So we are just using "master" and hoping for the best. | |||
apply from: 'https://github.com/mozilla-mobile/android-components/raw/master/components/service/glean/scripts/sdk_generator.gradle' | |||
apply from: 'https://github.com/mozilla-mobile/android-components/raw/master/components/service/glean/scripts/sdk_generator.gradle' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: EOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that should be addressed? I think that *nix prefers there to be an EOL at EOF, but Windows doesn't care. See this point.
Mildly-relevant side-note: some of my tools automatically add a line to EOF, and I don't want to hunt them down and disable 'em :P
Are you ok with leaving this in?
automation/taskcluster/lib/tasks.py
Outdated
@@ -409,17 +415,17 @@ def craft_nightly_signing_task( | |||
'machine': { | |||
'platform': 'android-all', | |||
}, | |||
'symbol': 'Ns', | |||
'symbol': '{}s'.format(capitalized_build_type[0]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, beta
would show up as Bs
, right? I don't mean to BS you, but that symbol is already used in mozilla-central for build-signing
. I'd prefer to not overload that symbol, if people familiar with Treeherder on m-c go to Fenix.
How about BeS
or beta-s
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beta-s
sounds great!
2dbc325
to
618fa05
Compare
aab01a8
to
fb8f3a4
Compare
8386d19
to
b457f7a
Compare
1274079
to
e60941d
Compare
This depends on:
Pull Request checklist