Closed Bug 1355482 Opened 3 years ago Closed 10 months ago

document taskcluster releng implementation and release promotion

Categories

(Release Engineering :: Documentation, enhancement, P2)

enhancement

Tracking

(firefox-esr60 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- fixed

People

(Reporter: kmoir, Assigned: aki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

10.69 KB, patch
bhearsum
: review+
dustin
: feedback+
Details | Diff | Splinter Review
45 bytes, text/x-phabricator-request
bhearsum
: review+
Details | Review
7.59 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
45 bytes, text/x-phabricator-request
bhearsum
: review+
Details | Review
59 bytes, text/x-review-board-request
bhearsum
: review+
Details
9.70 KB, patch
catlee
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
aki
: review+
Details
Each time I've met with the buildduty folks for the past weeks they have expressed that the lack of documentation on the current state of the tascluster migration and release promotion is very frustrating for them.  They can't find documentation to gain a better understanding of these systems in transition work.  

We need to create some docs for them so they can understand how the tasks and jobs are transformed, the kind dependencies, and any other knowledge they need so they can address get up to speed to address operational issues when they arise.

Topics include
beetmover
balrog
beetmover
l10n
signing
repackaging
transforms
kinds

release promotion
various steps in the release process, how we go from a ci build to a final release

In addition to documentation, it would be great if various releng folks could record tech talks on their area of expertise in this area. We could use these to onboard new employees/interns.
Kim, who do you think is best suited to do this? I'm thinking some combination of you, Mihai, Callek and Rail.
Flags: needinfo?(kmoir)
Priority: -- → P2
Sure, I think if if we each tackle a couple of topics this would be great.  Will bring it up at the next tcmigration meeting.  As well not sure where it should reside in releng docs or wiki, but pointers to content from the builduty wiki in any case would be a good start.
Flags: needinfo?(kmoir)
https://trello.com/c/yJ8lOHfA/205-expand-taskcluster-relpro-in-tree-docs specifies adding release promotion documentation in-tree, so it'll show up in the gecko rtd. We could use that for general docs and have supplemental buildduty docs in a wiki, or include buildduty docs in the same in-tree location.
Summary: document taskcluster releng implementation and release promotion for operational teams → document taskcluster releng implementation and release promotion
I'm starting some in-tree sphinx docs for tc relpro; hopefully this serves as a good starting point to add more docs.

To generate the gecko Sphinx docs locally,

    ./mach doc --outdir docs-out

and then you can browse to file:///.../docs-out/html/main/index.html

I had to install jsdoc before this would work... for mac, `brew install jsdoc3` . The error message from mach suggests using npm.

`./mach doc` failed initially because I had some old trees cloned inside of `testing/mozharness/build/mozilla-{beta,central}` (probably for merge day testing); evidently it scours the entire tree recursively for files to build. To fix, I moved the temp workdir `testing/mozharness/build` outside of my tree.
Keywords: leave-open
Using a branch off gecko-dev is probably even easier, since the rst is easily viewable as html on github.
First pass. This lays down structure that we can add to (e.g. https://trello.com/c/yJ8lOHfA/205-expand-taskcluster-relpro-in-tree-docs).
Attachment #8960792 - Flags: review?(bhearsum)
Attachment #8960792 - Flags: feedback?(dustin)
Added some more ascii art + clarifications.
Attachment #8960792 - Attachment is obsolete: true
Attachment #8960792 - Flags: review?(bhearsum)
Attachment #8960792 - Flags: feedback?(dustin)
Attachment #8961170 - Flags: review?(bhearsum)
Attachment #8961170 - Flags: feedback?(dustin)
Comment on attachment 8961170 [details] [diff] [review]
initial relpro in-tree documentation

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

This looks good, I really like the structure. There's a few notes below, mostly requests for expanding things a bit.

::: taskcluster/docs/release-promotion-action.rst
@@ +2,5 @@
> +========================
> +
> +The `release promotion action`_ allows us to chain multiple graphs together.
> +Essentially, we're using :ref:`optimization` logic to replace task labels in the
> +current graph with task IDs from the previous graph(s).

I know we in RelEng use the word "graph" a lot, but I wonder if we should start trying to switch to "group" (since that's the Taskcluster vernacular these days).

@@ +57,5 @@
> +|                  l10n-repack
> +|                       |
> +|                  l10n-signing
> +
> +(This is the ``snowman`` model: If you request the body of the snowman and

I think it would be good to make the snowman model explanation a bit more prominent (eg: not behind paranthesis, maybe also above the examples?) -- this metaphor seems to be working well when explaining it to new people.

@@ +72,5 @@
> +
> +Release promotion action mechanics
> +----------------------------------
> +
> +The action downloads the ``parameters.yml`` from the initial ``previous_graph_id``.

Can you add something that describes previous_graph_id (and any other action inputs) in more detail? When I read this paragraph I feel like I'm supposed to know something about it already - in particular, what the difference between an "initial" previous graph id and other ones is.

@@ +99,5 @@
> +------------------------------------------------------
> +
> +Currently, we're able to trigger this action via `Treeherder`_; we sometimes use this method for testing purposes. This requires being signed in with the right scopes. On `Release Promotion Projects`_, here's a dropdown in the top right of a given revision. Choose ``Custom Push Action``, then ``Release Promotion``. The inputs are specifiable as raw yaml on the left hand column.
> +
> +Triggering the release promotion action via releaserunner3

Since we have multiple ways to trigger these actions, it would be helpful to explain why someone would choose one over the other. (As things read now, I don't know why I would want to use releaserunner3 or trigger_action.py when I could just click some buttons on Treeherder.)

::: taskcluster/docs/release-promotion.rst
@@ +1,4 @@
> +Release Promotion
> +=================
> +
> +Release promotion allows us to ship the same compiled binaries that we've

This is an excellent intro/history lesson.
Attachment #8961170 - Flags: review?(bhearsum) → review+
Awesome. Now we have todos for the following additional docs:

* best practices in taskgraph kinds/transforms
* explain our kinds for people not in releng
* beetmover/balrog/partners/partials/signing/pushapk in depth

All up for grabs!
Hm, I thought I responded or had a draft of responses, but that tab seems to be missing.

(In reply to Ben Hearsum (:bhearsum) from comment #8)
> Comment on attachment 8961170 [details] [diff] [review]
> initial relpro in-tree documentation
> 
> Review of attachment 8961170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, I really like the structure. There's a few notes below,
> mostly requests for expanding things a bit.

Thanks! I addressed your review comments before landing. We can land followups if/as needed.

> ::: taskcluster/docs/release-promotion-action.rst
> @@ +2,5 @@
> > +========================
> > +
> > +The `release promotion action`_ allows us to chain multiple graphs together.
> > +Essentially, we're using :ref:`optimization` logic to replace task labels in the
> > +current graph with task IDs from the previous graph(s).
> 
> I know we in RelEng use the word "graph" a lot, but I wonder if we should
> start trying to switch to "group" (since that's the Taskcluster vernacular
> these days).

Done. We do have vestiges still, e.g. `previous_graph_ids` and `taskcluster/taskgraph`

> @@ +57,5 @@
> > +|                  l10n-repack
> > +|                       |
> > +|                  l10n-signing
> > +
> > +(This is the ``snowman`` model: If you request the body of the snowman and
> 
> I think it would be good to make the snowman model explanation a bit more
> prominent (eg: not behind paranthesis, maybe also above the examples?) --
> this metaphor seems to be working well when explaining it to new people.

Done. Glad to hear the analogy is helpful.

> @@ +72,5 @@
> > +
> > +Release promotion action mechanics
> > +----------------------------------
> > +
> > +The action downloads the ``parameters.yml`` from the initial ``previous_graph_id``.
> 
> Can you add something that describes previous_graph_id (and any other action
> inputs) in more detail? When I read this paragraph I feel like I'm supposed
> to know something about it already - in particular, what the difference
> between an "initial" previous graph id and other ones is.

Hm, I might have missed this comment in my followups. I can attach another patch.

> @@ +99,5 @@
> > +------------------------------------------------------
> > +
> > +Currently, we're able to trigger this action via `Treeherder`_; we sometimes use this method for testing purposes. This requires being signed in with the right scopes. On `Release Promotion Projects`_, here's a dropdown in the top right of a given revision. Choose ``Custom Push Action``, then ``Release Promotion``. The inputs are specifiable as raw yaml on the left hand column.
> > +
> > +Triggering the release promotion action via releaserunner3
> 
> Since we have multiple ways to trigger these actions, it would be helpful to
> explain why someone would choose one over the other. (As things read now, I
> don't know why I would want to use releaserunner3 or trigger_action.py when
> I could just click some buttons on Treeherder.)

Done.

> ::: taskcluster/docs/release-promotion.rst
> @@ +1,4 @@
> > +Release Promotion
> > +=================
> > +
> > +Release promotion allows us to ship the same compiled binaries that we've
> 
> This is an excellent intro/history lesson.

Thanks!
Pushed by asasaki@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dead3908b2cc
release promotion doc review followup. r=bhearsum
Comment on attachment 8961170 [details] [diff] [review]
initial relpro in-tree documentation

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

::: taskcluster/docs/release-promotion-action.rst
@@ +1,4 @@
> +Release Promotion Action
> +========================
> +
> +The `release promotion action`_ allows us to chain multiple graphs together.

The content of this section seems almost entirely focused on the mechanics of optimizing these promotion taskgraphs.  Maybe this would make a good subsection?  There's a need here for more general information: what does the release promotion action do? Who runs it?  Is there a different action for each phase of release promotion?  What are the parameters?  Much of that is answered below, so perhaps this just means re-ordering the content in this file.

@@ +2,5 @@
> +========================
> +
> +The `release promotion action`_ allows us to chain multiple graphs together.
> +Essentially, we're using :ref:`optimization` logic to replace task labels in the
> +current graph with task IDs from the previous graph(s).

Within the in-tree configuration, taskgraph is the right term.

@@ +10,5 @@
> +    G
> +    |
> +    t1
> +    |
> +    t2

This looks like G (a graph) depends on t1 -- is G meant to denote the decision task here?  Or is that intended as a label?

  G

  t1
  |
  t2

@@ +27,5 @@
> +    or
> +    
> +    G1        G2        G
> +    |         |         |
> +    t1        t0        |

Why is this t0 here?

@@ +97,5 @@
> +
> +Triggering the release promotion action via Treeherder
> +------------------------------------------------------
> +
> +Currently, we're able to trigger this action via `Treeherder`_; we sometimes use this method for testing purposes. This requires being signed in with the right scopes. On `Release Promotion Projects`_, here's a dropdown in the top right of a given revision. Choose ``Custom Push Action``, then ``Release Promotion``. The inputs are specifiable as raw yaml on the left hand column.

*there's

@@ +104,5 @@
> +----------------------------------------------------------
> +
> +`Releaserunner3`_ is our current method of triggering the release promotion action in production. Examples of how to run this are in the `releasewarrior docs`_.
> +
> +To deal with the above ``previous_graph_ids`` logic, we allow for a ``decision_task_id`` in `trigger_action.py`_. As of 2018-03-14, this script assumes we want to download ``parameters.yml`` from the same decision task that we get ``actions.json`` from.

Is there a need to include the date here?  Are older versions of releaserunner3 still used?

::: taskcluster/docs/release-promotion.rst
@@ +6,5 @@
> +
> +In the olden days, we used to re-compile our release builds with separate
> +configs, which led to release-specific bugs which weren't caught by continuous
> +integration tests. This also meant we required new builds at release time, which
> + also increased the end-to-end time for a given release significantly. Release

too many "also"
leading ' ' in this line
Attachment #8961170 - Flags: feedback?(dustin) → feedback+
Pushed by asasaki@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af32770a782b
relpro docs: address dustin's review comments. r=dustin
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #14)
> Comment on attachment 8961170 [details] [diff] [review]
> initial relpro in-tree documentation

Thanks!

> ::: taskcluster/docs/release-promotion-action.rst
> @@ +1,4 @@
> > +Release Promotion Action
> > +========================
> > +
> > +The `release promotion action`_ allows us to chain multiple graphs together.
> 
> The content of this section seems almost entirely focused on the mechanics
> of optimizing these promotion taskgraphs.  Maybe this would make a good
> subsection?  There's a need here for more general information: what does the
> release promotion action do? Who runs it?  Is there a different action for
> each phase of release promotion?  What are the parameters?  Much of that is
> answered below, so perhaps this just means re-ordering the content in this
> file.

I put some overview in the other page, but it makes sense to put an
action-specific overview here. Done.

> @@ +2,5 @@
> > +========================
> > +
> > +The `release promotion action`_ allows us to chain multiple graphs together.
> > +Essentially, we're using :ref:`optimization` logic to replace task labels in the
> > +current graph with task IDs from the previous graph(s).
> 
> Within the in-tree configuration, taskgraph is the right term.

Ok. After Catlee's comment, I replaced "graph" with "task group" in most places, noting that "graph" was an alternate term. I think I'll go back over and rename to "taskgraph" and note that "task group" and "graph" are alternate terms.

> @@ +10,5 @@
> > +    G
> > +    |
> > +    t1
> > +    |
> > +    t2
> 
> This looks like G (a graph) depends on t1 -- is G meant to denote the
> decision task here?  Or is that intended as a label?

It's both the decision/action task as well as the graph label. From the above line:

    `Let's call our new graph ``G``::`

I'm certain we can make this better, but I wasn't sure how best to do that.

> @@ +27,5 @@
> > +    or
> > +    
> > +    G1        G2        G
> > +    |         |         |
> > +    t1        t0        |
> 
> Why is this t0 here?

To show that tasks outside of graph G aren't used in the optimization.
I could have filled in a few more screens full of different `previous_graph_ids`
examples; the ones I have were the minimum I thought could represent multiple
`previous_graph_ids` behavior.

> @@ +97,5 @@
> > +
> > +Triggering the release promotion action via Treeherder
> > +------------------------------------------------------
> > +
> > +Currently, we're able to trigger this action via `Treeherder`_; we sometimes use this method for testing purposes. This requires being signed in with the right scopes. On `Release Promotion Projects`_, here's a dropdown in the top right of a given revision. Choose ``Custom Push Action``, then ``Release Promotion``. The inputs are specifiable as raw yaml on the left hand column.
> 
> *there's

Fixed.

> @@ +104,5 @@
> > +----------------------------------------------------------
> > +
> > +`Releaserunner3`_ is our current method of triggering the release promotion action in production. Examples of how to run this are in the `releasewarrior docs`_.
> > +
> > +To deal with the above ``previous_graph_ids`` logic, we allow for a ``decision_task_id`` in `trigger_action.py`_. As of 2018-03-14, this script assumes we want to download ``parameters.yml`` from the same decision task that we get ``actions.json`` from.
> 
> Is there a need to include the date here?  Are older versions of
> releaserunner3 still used?

Future-proofing against rr3 changes that aren't reflected in the in-tree docs, since rr3 lives out-of-tree with no current plans to move it. Ideally we replace it all with ship-it v2, that removes the need to run `trigger_action.py` manually at all, and we can revamp this section.

> ::: taskcluster/docs/release-promotion.rst
> @@ +6,5 @@
> > +
> > +In the olden days, we used to re-compile our release builds with separate
> > +configs, which led to release-specific bugs which weren't caught by continuous
> > +integration tests. This also meant we required new builds at release time, which
> > + also increased the end-to-end time for a given release significantly. Release
> 
> too many "also"
> leading ' ' in this line

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af32770a782bc9a12fa4aa2e8b04379d84627e05
Attached patch add balrog kind documentation (obsolete) — Splinter Review
I think this covers enough? I couldn't think of anything else to add, or other places that would benefit from Balrog mentions.

I also removed some dead kinds (balrog-l10n, release-secondary-balrog-publishing).
Attachment #8966321 - Flags: review?(aki)
Comment on attachment 8966321 [details] [diff] [review]
add balrog kind documentation

Much better! Do you think it's worth linking to the official balrog docs + the balrog and balrogscript repos from http://gecko-docs.mozilla.org.s3.amazonaws.com/taskcluster/taskcluster/release-promotion.html somewhere?
Attachment #8966321 - Flags: review?(aki) → review+
I added the links you suggested, and was inspired by Simon's patch to make a higher level section, too.
Attachment #8966321 - Attachment is obsolete: true
Attachment #8966640 - Flags: review?(aki)
Comment on attachment 8966640 [details] [diff] [review]
updated balrog docs

Thank you!

>diff --git a/taskcluster/docs/release-promotion.rst b/taskcluster/docs/release-promotion.rst
>index e7f5470ccb3d..214a639ba7ce 100644
>--- a/taskcluster/docs/release-promotion.rst
>+++ b/taskcluster/docs/release-promotion.rst
>@@ -41,8 +41,12 @@ limited rollout percentage or are dependent on multiple downstream signoffs to
> fully ship.
> 
> In-depth relpro guide
> ---------------------
> 
> .. toctree::
> 
>     release-promotion-action
>+
>+Release Promotion Tasks
>+-----------------------
>+* :doc:`balrog`

I was thinking we'd add these subpages to the toctree. I'm ok with it here if you think it works better.
Attachment #8966640 - Flags: review?(aki) → review+
(In reply to Aki Sasaki [:aki] from comment #23)
> Comment on attachment 8966640 [details] [diff] [review]
> updated balrog docs
> 
> Thank you!
> 
> >diff --git a/taskcluster/docs/release-promotion.rst b/taskcluster/docs/release-promotion.rst
> >index e7f5470ccb3d..214a639ba7ce 100644
> >--- a/taskcluster/docs/release-promotion.rst
> >+++ b/taskcluster/docs/release-promotion.rst
> >@@ -41,8 +41,12 @@ limited rollout percentage or are dependent on multiple downstream signoffs to
> > fully ship.
> > 
> > In-depth relpro guide
> > ---------------------
> > 
> > .. toctree::
> > 
> >     release-promotion-action
> >+
> >+Release Promotion Tasks
> >+-----------------------
> >+* :doc:`balrog`
> 
> I was thinking we'd add these subpages to the toctree. I'm ok with it here
> if you think it works better.

Fine with me, I was just following the trend that Simon set in his partials documentation.
Pushed by bhearsum@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff3f4b6ce41
document taskcluster releng implementation and release promotion - add in depth balrog docs. r=aki
Comment on attachment 8966535 [details]
Bug 1355482 Partials documentation r=bhearsum

Ben Hearsum (:bhearsum) has approved the revision.

https://phabricator.services.mozilla.com/D898
Attachment #8966535 - Flags: review+
(In reply to Pulsebot from comment #25)
> Pushed by bhearsum@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff3f4b6ce41
> document taskcluster releng implementation and release promotion - add in
> depth balrog docs. r=aki

I forgot to add the new file in this. I'm pushing that now.
Pushed by bhearsum@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3030ca3e161
document taskcluster releng implementation and release promotion - add missing file. r=aki
A space was needed to make the code block show up
Comment on attachment 8968436 [details]
Bug 1355482 Fix partials documentation code block r=bhearsum

Ben Hearsum (:bhearsum) has approved the revision.

https://phabricator.services.mozilla.com/D957
Attachment #8968436 - Flags: review+
Pushed by sfraser@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d00dc13a563
Fix partials documentation code block r=bhearsum
Component: General → Documentation
Attachment #8980387 - Flags: review?(catlee)
Comment on attachment 8980387 [details] [diff] [review]
signing-docs.patch

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

Looks great, thanks for writing this up!

Just a few minor nits.

::: taskcluster/docs/signing.rst
@@ +66,5 @@
> +
> +**Build internal signing**: Certain package types require the internals to be signed.
> +Certain formats extract the internal binaries and sign them, e.g. exe or dmg.
> +These kinds include `build-signing`, `nightly-l10n-signing`,
> +`release-eme-free-repack-signing`, and `release-partner-repack-signing`.

I don't really understand this description. Is this for e.g. signing xul.dll on Windows?

@@ +110,5 @@
> +is a ``tar.gz``.
> +
> +``signcode`` signing takes individual binaries or a zipfile. We sign the
> +individual file or internals of the zipfile, skipping any already-signed files
> +and a select few blocklisted files (using the `_should_sign_windows`_ function).

s/blocklisted/blacklisted/

@@ +119,5 @@
> +``mar`` signing signs our update files (Mozilla ARchive). ``mar_sha384`` is
> +the same, but with a different hashing algorithm.
> +
> +``emevoucher`` - Firefox releases support `Encrypted Media Extensions`_ by default
> +(EME-free builds are available, however). The voucher is part of that.

I don't think this is used any more. Probably don't need to include it.
Attachment #8980387 - Flags: review?(catlee) → review+
(In reply to Chris AtLee [:catlee] from comment #40)
> Comment on attachment 8980387 [details] [diff] [review]
> signing-docs.patch
> 
> Review of attachment 8980387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, thanks for writing this up!
> 
> Just a few minor nits.
> 
> ::: taskcluster/docs/signing.rst
> @@ +66,5 @@
> > +
> > +**Build internal signing**: Certain package types require the internals to be signed.
> > +Certain formats extract the internal binaries and sign them, e.g. exe or dmg.
> > +These kinds include `build-signing`, `nightly-l10n-signing`,
> > +`release-eme-free-repack-signing`, and `release-partner-repack-signing`.
> 
> I don't really understand this description. Is this for e.g. signing xul.dll
> on Windows?

Correct. I'll try to word this better.

> @@ +110,5 @@
> > +is a ``tar.gz``.
> > +
> > +``signcode`` signing takes individual binaries or a zipfile. We sign the
> > +individual file or internals of the zipfile, skipping any already-signed files
> > +and a select few blocklisted files (using the `_should_sign_windows`_ function).
> 
> s/blocklisted/blacklisted/

I'm explicitly using `blocklisted` and `allowlisted` rather than `blacklisted` and `whitelisted`.

> @@ +119,5 @@
> > +``mar`` signing signs our update files (Mozilla ARchive). ``mar_sha384`` is
> > +the same, but with a different hashing algorithm.
> > +
> > +``emevoucher`` - Firefox releases support `Encrypted Media Extensions`_ by default
> > +(EME-free builds are available, however). The voucher is part of that.
> 
> I don't think this is used any more. Probably don't need to include it.

Ok!
Comment on attachment 8980360 [details]
Bug 1355482 - Add pushapk documentation

https://reviewboard.mozilla.org/r/246536/#review252836

Looks good overall! A few suggestions below:

::: taskcluster/docs/pushapk.rst:18
(Diff revision 1)
> +The google-play-strings downloads every string for every locale from the `l10n website`_. This happens at the very beginning of the promotion, in order to spot any potential issue (like infrastructure-related), early. This prevents holding the release at the last minute .
> +
> +Push it all to Google Play
> +--------------------------
> +
> +Before any request is made to Google Play, the push-apk task sanity check the data passed over:

Grammar fix: "sanity checks"

::: taskcluster/docs/pushapk.rst:25
(Diff revision 1)
> +1. APKs are signed with the correct certificates. This may not sound extremely important because Google Play is vigilant about APK signatures and will refuse any APK for which the signature is not valid. However, it is safer to bail out before any outbound traffic is done to Google Play. Besides, with this check, Google acts as a second factor instead of being the only actor accountable for signatures.
> +2. All given APKs have matching data (version numbers, package names, etc.)
> +3. No required processor architecture is missing, in order to upload them all in the same request. We have to publish them at the same time because some Android devices support several architectures. We have already had one big crash on these devices because an x86 APK was overseeded by its “brother in ARM”.
> +4. APKs are multilocales
> +
> +Then it uploads the APKs and the strings fetched in the previous task. Like said above, Google Play performs some additional checks on their end. These errors are reported back.

It might be helpful here to give a few examples about the types of checks that Google Play does (if we even know).

::: taskcluster/docs/pushapk.rst:29
(Diff revision 1)
> +
> +Then it uploads the APKs and the strings fetched in the previous task. Like said above, Google Play performs some additional checks on their end. These errors are reported back.
> +
> +These checks and actions are done in several components. Here's how they interact:
> +
> +.. image:: https://johanlorenzo.github.io/blog/images/posts/push-apk/architecture.svg

This graph is now part of the docs! You should check it in.

::: taskcluster/docs/pushapk.rst:29
(Diff revision 1)
> +
> +Then it uploads the APKs and the strings fetched in the previous task. Like said above, Google Play performs some additional checks on their end. These errors are reported back.
> +
> +These checks and actions are done in several components. Here's how they interact:
> +
> +.. image:: https://johanlorenzo.github.io/blog/images/posts/push-apk/architecture.svg

This graph looks good overall, I just have one question: What system is "stores_l10n" part of (I'm guessing that's the task that has previously downloaded and stored the l10n strings)? I think it could use an encompassing box, a more descriptive name, or both.
Comment on attachment 8980360 [details]
Bug 1355482 - Add pushapk documentation

https://reviewboard.mozilla.org/r/246536/#review253570
Attachment #8980360 - Flags: review?(bhearsum) → review-
Comment on attachment 8980360 [details]
Bug 1355482 - Add pushapk documentation

https://reviewboard.mozilla.org/r/246536/#review254726

::: taskcluster/docs/pushapk.rst:25
(Diff revision 1)
> +1. APKs are signed with the correct certificates. This may not sound extremely important because Google Play is vigilant about APK signatures and will refuse any APK for which the signature is not valid. However, it is safer to bail out before any outbound traffic is done to Google Play. Besides, with this check, Google acts as a second factor instead of being the only actor accountable for signatures.
> +2. All given APKs have matching data (version numbers, package names, etc.)
> +3. No required processor architecture is missing, in order to upload them all in the same request. We have to publish them at the same time because some Android devices support several architectures. We have already had one big crash on these devices because an x86 APK was overseeded by its “brother in ARM”.
> +4. APKs are multilocales
> +
> +Then it uploads the APKs and the strings fetched in the previous task. Like said above, Google Play performs some additional checks on their end. These errors are reported back.

Good idea. I added 2 checks I remember we hit.

::: taskcluster/docs/pushapk.rst:29
(Diff revision 1)
> +
> +Then it uploads the APKs and the strings fetched in the previous task. Like said above, Google Play performs some additional checks on their end. These errors are reported back.
> +
> +These checks and actions are done in several components. Here's how they interact:
> +
> +.. image:: https://johanlorenzo.github.io/blog/images/posts/push-apk/architecture.svg

Good point. I added the graph in tree.

I changed the schema to be less confusing about "stores_l10n" That was the way we did, before strings are fetched in a separate task. What do you think?
Comment on attachment 8980360 [details]
Bug 1355482 - Add pushapk documentation

https://reviewboard.mozilla.org/r/246536/#review254750
Attachment #8980360 - Flags: review?(bhearsum) → review+
Duplicate of this bug: 1471493
Comment on attachment 8988123 [details]
Bug 1355482 - documentation cleanups for release promotion,

https://reviewboard.mozilla.org/r/253372/#review259992


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/util/partners.py:63
(Diff revision 1)
> +# {
> +#   "data": {
> +#     "repository": {
> +#       "object": {
> +#         "text": "<?xml version=\"1.0\" ?>\n<manifest>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla-partners/\" name=\"mozilla-partners\"/>\n  " +

Error: Line too long (104 > 99 characters) [flake8: E501]

::: taskcluster/taskgraph/util/partners.py:65
(Diff revision 1)
> +#     "repository": {
> +#       "object": {
> +#         "text": "<?xml version=\"1.0\" ?>\n<manifest>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla-partners/\" name=\"mozilla-partners\"/>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla/\" name=\"mozilla\"/>\n\n  " +
> +#                 "<project name=\"repack-scripts\" path=\"scripts\" remote=\"mozilla-partners\" " +

Error: Line too long (100 > 99 characters) [flake8: E501]

::: taskcluster/taskgraph/util/partners.py:66
(Diff revision 1)
> +#       "object": {
> +#         "text": "<?xml version=\"1.0\" ?>\n<manifest>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla-partners/\" name=\"mozilla-partners\"/>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla/\" name=\"mozilla\"/>\n\n  " +
> +#                 "<project name=\"repack-scripts\" path=\"scripts\" remote=\"mozilla-partners\" " +
> +#                 "revision=\"master\"/>\n  <project name=\"build-tools\" path=\"scripts/tools\" " +

Error: Line too long (100 > 99 characters) [flake8: E501]

::: taskcluster/taskgraph/util/partners.py:67
(Diff revision 1)
> +#         "text": "<?xml version=\"1.0\" ?>\n<manifest>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla-partners/\" name=\"mozilla-partners\"/>\n  " +
> +#                 "<remote fetch=\"git@github.com:mozilla/\" name=\"mozilla\"/>\n\n  " +
> +#                 "<project name=\"repack-scripts\" path=\"scripts\" remote=\"mozilla-partners\" " +
> +#                 "revision=\"master\"/>\n  <project name=\"build-tools\" path=\"scripts/tools\" " +
> +#                 "remote=\"mozilla\" revision=\"master\"/>\n  <project name=\"mozilla-EME-free\" " +

Error: Line too long (101 > 99 characters) [flake8: E501]
Comment on attachment 8988122 [details]
Bug 1355482 - documentation for partner repacks,

https://reviewboard.mozilla.org/r/253370/#review260122

::: taskcluster/docs/partner-repacks.rst:30
(Diff revision 1)
> +* ``release_enable_partners``
> +* ``release_partner_config``
> +* ``release_partner_build_number``
> +* ``release_partners``
> +
> +We split the repacks into two 'branches', EME-free and everything else, to retain some

I wonder if the term `branches` might confuse people into thinking they're on separate vcs branches. I'm not sure how best to improve on it: two sets of kinds? Explicitly note that these are branches of the graph, rather than repo branches?

::: taskcluster/docs/partner-repacks.rst:34
(Diff revision 1)
> +
> +We split the repacks into two 'branches', EME-free and everything else, to retain some
> +flexibility over enabling/disabling them separately. This costs us some duplication of the kinds
> +in the repacking stack. The two enable parameters are booleans to turn these two branches
> +on/off. We set them in release-runner3's `is_partner_enabled() <https://dxr.mozilla
> +.org/build-central/source/tools/buildfarm/release/release-runner3.py#144>`_ when starting a release.

We'll need to update this link when we switch to ship-it v2.

An aside on these links: because they're not pinned to a revision, the line numbers may become inaccurate over time. If we link to a specific revision, the logic may become outdated over time. I'm not sure how best to resolve this.
Attachment #8988122 - Flags: review?(aki) → review+
Comment on attachment 8988123 [details]
Bug 1355482 - documentation cleanups for release promotion,

https://reviewboard.mozilla.org/r/253372/#review260134

Thanks!
Attachment #8988123 - Flags: review?(aki) → review+
Attachment #8988123 - Attachment is obsolete: true
Comment on attachment 8988122 [details]
Bug 1355482 - documentation for partner repacks,

https://reviewboard.mozilla.org/r/253370/#review260122

> I wonder if the term `branches` might confuse people into thinking they're on separate vcs branches. I'm not sure how best to improve on it: two sets of kinds? Explicitly note that these are branches of the graph, rather than repo branches?

I improved this by switching to dxr search links which redirect to the result.
Pushed by nthomas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8ebfd75f501
documentation for partner repacks, r=aki
Pushed by nthomas@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/347196a3c2c0
documentation cleanups for release promotion, r=aki
(In reply to Nick Thomas [:nthomas] (UTC+12) from comment #58)
> Comment on attachment 8988123 [details]
> Bug 1355482 - documentation cleanups for release promotion,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/253372/diff/1-2/

I somehow managed to mark this is as discarded in mozreview, but landed it at https://hg.mozilla.org/integration/mozilla-inbound/rev/347196a3c2c004364d07bd1d6a8e1a94dbcb3fb2
I think this is good. We can continue to add more, e.g. bouncer.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Assignee: nobody → nthomas
This was a bit of a group effort but aki did the most.
Assignee: nthomas → aki
You need to log in before you can comment on or make changes to this bug.