Add comments to note that Adjust beta builds in automation use sandbox token rather than beta token

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 50
All
Android
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

Test failures in bug 1277553 discovered that we unexpectedly use the adjust sandbox token for mozilla-beta tree builds on treeherder (when we should be using the beta key). Ideally, we should minimize the differences between our released builds & our test builds so we should make the treeherder beta builds use the beta token.

Note that it is only after bug 1275669 that we don't make pings from automation so we don't skew our results.
Desktop goes through a process called "release promotion" where builds from CI are taken and used for release builds. If mobile moves to this process I expect Adjust to break.

Since that makes this more important, assigning self so it doesn't get lost, but I'd prefer to have the help of the build team here.
---

We set the which token each build uses in mobile/android/config/mozconfigs/common by checking the MOZ_UPDATE_CHANNEL flag:
  https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/mobile/android/config/mozconfigs/common#55

I'm guessing this flag is never set in CI builds (e.g. perhaps because the updater is not enabled) but is set for our release builds.

Chris, is there a better value we can use to determine which release channel we're on in a mozconfig? Or perhaps we're not doing this correctly altogether?
Assignee: nobody → michael.l.comella
Flags: needinfo?(cmanchester)
Spoke with Chris irl.

(In reply to Michael Comella (:mcomella) from comment #1)
> Desktop goes through a process called "release promotion" where builds from
> CI are taken and used for release builds. If mobile moves to this process I
> expect Adjust to break.
> 
> Since that makes this more important, assigning self so it doesn't get lost,
> but I'd prefer to have the help of the build team here.

Chris said I shouldn't assume this will break – mobile needs to have MOZ_UPDATE_CHANNEL for release builds so that value should be set in automation if we use the automation builds for release (which is likely what desktop does).

Otherwise, Chris mentioned that he doesn't know of a way off the top of his head to do this on Android. Desktop, on the other hand, can use their separate release/beta mozconfigs for this.

That being said, since this probably won't break with release promotion and it's non-trivial to implement this, I'm going to say we shouldn't invest the time into it.

However, let's add a warning about what's going on to the comments in the build file.
Flags: needinfo?(cmanchester)
Summary: Adjust beta builds use sandbox token rather than beta token → Add comments to note that Adjust beta builds in automation use sandbox token rather than beta token
Created attachment 8766587 [details]
Bug 1283318 - Improve comments for adjust keyfile build config.

Review commit: https://reviewboard.mozilla.org/r/61424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61424/
Attachment #8766587 - Flags: review?(cmanchester)
Relevant info:

(In reply to Michael Comella (:mcomella) from bug 1277553 comment #31)
> After investigation in [this bug], I think the problem is actually that
> MOZ_UPDATE_CHANNEL is not specified for *all* CI builds (as opposed to
> release builds) and that RyanVM's patches are not special. I added a comment
> to the build files to this effect in [this] bug.
Comment on attachment 8766587 [details]
Bug 1283318 - Improve comments for adjust keyfile build config.

https://reviewboard.mozilla.org/r/61424/#review58446

::: mobile/android/config/mozconfigs/common:56
(Diff revision 1)
> +# Note: bug 1275669 adds code to prevent adjust pings in CI.
> +#
> +# MOZ_UPDATE_CHANNEL is only specified for release builds and not CI (e.g.
> +# treeherder) builds, even on the beta & release channels. Therefore, all CI
> +# builds are expected to use the default keyfile.
> +#
> +# Even though we don't upload in automation, the default keyfile is necessary
> +# because the release & beta release/CI builds specify MOZ_INSTALL_TRACKING &
> +# an assertion is thrown if a keyfile is not specified with that env var
> +# specified.
> +#
> +# Ideally, CI builds would set MOZ_UPDATE_CHANNEL & we could remove the default
> +# keyfile. This may occur if fennec moves to the release promotion model.
> +#
> +# Having a default keyfile has the disadvantage that if our beta/release checks
> +# ever fail, we'll come to the default case and use the sandbox token instead
> +# of asserting that no adjust token is specified. I don't have any better
> +# alternatives at the moment.

I think this comment is a little long winded, and likely to bitrot if/when release promotion is implemented for Android builds, but I'll leave that to your judgement.

The two points I would emphasize are that MOZ_INSTALL_TRACKING doesn't always guarantee MOZ_UPDATE_CHANNEL is set to release or beta, and that we prevent pings from automation independent of what happens here.
Attachment #8766587 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester (:chmanchester) from comment #5)
> I think this comment is a little long winded, and likely to bitrot if/when
> release promotion is implemented for Android builds, but I'll leave that to
> your judgement.

I agree but I believe there is useful info in there (e.g. why is the assertion useful?). With your suggestions, I rewrote the comment to be less bit-rottable & better organized, though still a bit lengthy.

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6038eb739fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.