land firefox+devedition relpro patches from maple to m-c

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: aki, Assigned: aki)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(34 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
59 bytes, text/x-review-board-request
Callek
: review+
Details
We have a lot of unmerged patches, largely due to both the change freeze and not wanting to have patches ride the trains before they're ready. It's probably too late at this time to have bisectable, logically split patches. At the moment I'm thinking we'll split these up by file; I'll try to get the base files early in the review, and add the relatively simple-to-review kind yml files at the end.

Starting these reviews early, even though we're expecting more changes, so we have more time to address review comments. Thanks for the suggestion, Callek!

The patches in this bug are dependent on bug 1423014. I'm also excluding Johan's google play work in bug 1385401.
I currently have https://github.com/mozilla/gecko-dev/compare/master...escapewindow:maple-staging?expand=1 . I'm hoping to clean up the commit comments and possibly rebase a bit more for sanity before submitting to mozreview.
Blocks: 1397773
Comment on attachment 8934790 [details]
bug 1423081 - add release partials support.

https://reviewboard.mozilla.org/r/205668/#review211640

This one looks pretty good, I'm not marking r+ yet until I get a better feel for the whole patchset and these questions though.

::: taskcluster/ci/partials-signing/kind.yml:17
(Diff revision 1)
>  kind-dependencies:
>    - partials
> +
> +job-template:
> +  shipping-phase: promote
> +  shipping-product: firefox

this should support devedition, for shipping-product, right?

(I'm assuming that is upcoming, leaving this as an issue to confirm... I won't be marking other cases of this potential for now)

::: taskcluster/taskgraph/transforms/partials.py:109
(Diff revision 1)
> +            if 'product' in builds[build]:
> +                partial_info['product'] = builds[build]['product']
> +            if 'previousVersion' in builds[build]:
> +                partial_info['previousVersion'] = builds[build]['previousVersion']
> +            if 'previousBuildNumber' in builds[build]:
> +                partial_info['previousBuildNumber'] = builds[build]['previousBuildNumber']

I'm not sure I understand what these previous* things are used for. I could use some help there...  (since they are passed as data into the generation of partials for funsize)

::: taskcluster/taskgraph/util/partials.py:165
(Diff revision 1)
> +        if "-localtest" in channel:
> +            return channel
> +
> +
> +def populate_release_history(product, branch, maxbuilds=4, maxsearch=10,
> +                             partial_updates=None):

I don't see partial_updates being utilized from the decision graph itself.

Is it intentional that we don't seem to have any way to fill that in other than via action tasks?
Comment on attachment 8934802 [details]
bug 1423081 - buildbot release worker doesn't need force=True.

https://reviewboard.mozilla.org/r/205692/#review211650

::: commit-message-47d48:1
(Diff revision 1)
> +bug 1423081 - buildbot release worker doesn't need force=True. r=callek

Technically, per current maple anyway, get_release_config doesn't even take a kwarg of force anymore ;-)
Attachment #8934802 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934801 [details]
bug 1423081 - add devedition+source to the gecko_v2_whitelist.

https://reviewboard.mozilla.org/r/205690/#review211652

::: commit-message-88265:1
(Diff revision 1)
> +bug 1423081 - add devedition+source to the gecko_v2_whitelist. r=callek

This patch makes me wonder if "{}-devedition" can be removed in favor of the "{}-devedition-opt" entries. (we'll probably just drop the whitelist once bb is dead, so not a requirement)
Attachment #8934801 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934812 [details]
bug 1423081 - add version.txt to decision sparse checkout.

https://reviewboard.mozilla.org/r/205712/#review211654
Attachment #8934812 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934823 [details]
bug 1423081 - desktop tc relpro taskcluster docs.

https://reviewboard.mozilla.org/r/205734/#review211656

::: taskcluster/docs/attributes.rst:196
(Diff revision 1)
>  ================
>  For release promotion jobs, this is the product we are shipping.
>  
>  shipping_phase
>  ==============
> -For release promotion jobs, this is the shipping phase (promote, publish, ship).
> +For release promotion jobs, this is the shipping phase (build, promote, push, ship).

nit: would love a longer description of what each phase is, so that future work has a definition of what belongs in each phase. 

We can postpone that for "cleanup" though.

::: taskcluster/docs/kinds.rst:206
(Diff revision 1)
>  Beetmover-cdns publishes promoted releases to CDNs. This is part of release promotion.
>  
> +beetmover-source
> +-------------------
> +
> +Beetmover-source publishes release source This is part of release promotion.

s/release source This/release source. This/

::: taskcluster/docs/kinds.rst:234
(Diff revision 1)
>  all the signed multi-locales (aka "multi") APKs for a given release and upload them
>  all at once. They also depend on the breakpoint.
>  
> -release-notify-publish
> +release-binary-transparency
> +---------------------------
> +Binary transparency to issue a certificate

I think many of these release-* kind descriptions are a bit under-verbose, e.g. "What is binary transparency, what sort of certificate..." and "What is a snap"  --> "Task that generates an installer using Ubuntu's Snap format"

This is more cleanup fodder though

::: taskcluster/docs/kinds.rst:337
(Diff revision 1)
> +--------------------
> +Dummy tasks to handle balrog tasks as dependencies without hitting max-dependencies.
> +
> +post-beetmover-dummy
> +--------------------
> +Dummy tasks to handle beetmover tasks as dependencies without hitting max-dependencies.

nit (x2) "Dummy tasks to consolidate .... dependencies used in order to avoid taskcluster limits on number of dependencies per task" or some such.
Attachment #8934823 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

::: taskcluster/taskgraph/transforms/task.py:362
(Diff revision 1)
>          Optional('command'): [taskref_or_string],
>  
>          # the maximum time to run, in seconds
>          Required('max-run-time'): int,
>  
>          # the exit status code that indicates the task should be retried

nit: update the comment to say that an array of exit status codes is ok.

::: taskcluster/taskgraph/transforms/task.py:525
(Diff revision 1)
> +        Required('contact', default=None): optionally_keyed_by(
> +            'project', 'product', Any(None, basestring)
> +        ),
> +
> +        # the maximum time to run, in seconds
> +        Required('max-run-time', default=600): int,

None of these other entries in the worker are utilized from any passed down tasks in maple.

Are they required to be configurable or should we hardcode in the payload builder below to cut out complexity for now?

::: taskcluster/taskgraph/transforms/task.py:736
(Diff revision 1)
>          key=key
>      )
>  
>  
>  def verify_index_job_name(index):
> -    job_name = index['job-name']
> +    job_name = index.get('job-name')

what cases will we not have a job-name and still be valid?

::: taskcluster/taskgraph/transforms/task.py:999
(Diff revision 1)
> +    worker = task['worker']
> +    release_config = get_release_config(config)
> +
> +    contact = resolve_keyed_by(
> +        worker, 'contact', 'binary-transparency',
> +        project=config.params['project'],

As mentioned above this doesn't actually seem used by entries here.

Of note of course, is that the dict of the job is passed by reference (not copy) [unless something changed outside of my memory] and that resolve_keyed_by replaces values.

So if you're not doing a .deepcopy() you run the risk of having the return of this resolve not being valid against any other possible returns from the same taskgraph. (as in, if you resolve by product, whichever product gets run first will be sticky and you won't get the resolve for next product).

Though as I said, nothing in the current maple branch uses this param, so you get metadata.owner anyway instead ;-)

::: taskcluster/taskgraph/transforms/task.py
(Diff revision 1)
>  def add_release_index_routes(config, task):
>      index = task.get('index')
>      routes = []
> -    release_config = get_release_config(config, force=True)
> +    release_config = get_release_config(config)
> -
> -    verify_index_job_name(index)

why don't we want to verify here anymore?

::: taskcluster/taskgraph/transforms/task.py:1349
(Diff revision 1)
>          worker_type = task['worker-type'].format(level=level)
>          provisioner_id, worker_type = worker_type.split('/', 1)
> +        project = config.params['project']
>  
>          routes = task.get('routes', [])
> -        scopes = [s.format(level=level) for s in task.get('scopes', [])]
> +        scopes = [s.format(level=level, project=project) for s in task.get('scopes', [])]

Dustin has once scolded me for doing this ({project}) replacement in transforms. It is why I have a big warning in l10n.py transforms [1]

Can we do some sort of comment here that expanding scope on project is only for buildbot-bridge or something. (or drop it entirely if we don't actually need it)

[1] - https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/l10n.py#344

::: taskcluster/taskgraph/transforms/task.py:1461
(Diff revision 1)
> -        attributes['shipping_phase'] = task['shipping-phase']
> +            attributes['shipping_phase'] = task['shipping-phase']
> -        attributes['shipping_product'] = task['shipping-product']
> +        else:
> +            attributes.setdefault('shipping_phase', None)
> +        # shipping_product will always match the upstream task's
> +        # shipping_product, so a pre-set existing attributes['shipping_product']
> +        # takes precedence over task['shipping-product'].

I'd rather we have a sanity check/exception here if task['shipping-product'] !== attributes['shipping_product'] when set.

Since doing otherwise would cause confusion in the job-defaults and such. (It may have caused me some confusion in some already-looked at reviews).

::: taskcluster/taskgraph/transforms/task.py:1490
(Diff revision 1)
> -                if v is None:
> -                    continue
> -                elif isinstance(v, list):
> -                    v = {'ids': v}
> -                    if 'completed' == k:
> -                        v.update({
> +                    resolve_keyed_by(
> +                        notification,
> +                        notification_option,
> +                        'notifications',
> +                        project=config.params['project'],
> +                    )

same comment as above re: resolve_keyed_by

::: taskcluster/taskgraph/transforms/task.py:1513
(Diff revision 1)
> +                # change event to correct event
> +                if notification_event != 'artifact-created':
> +                    notification_event = 'task-' + notification_event
> +
> +                # update notifications
> +                task_def['extra']['notifications'][notification_event] = notification

Notification stuff is a bit hard to reason about here, and increases the size of this function and with it cognitive load...

I'd love if we either made a seperate transform in this file, or a seperate transform file itself to support notification stuff.

This can be relegated to followup.
Comment on attachment 8934792 [details]
bug 1423081 - source readme.

https://reviewboard.mozilla.org/r/205672/#review211942

::: taskcluster/taskgraph/transforms/job/mozharness.py:146
(Diff revision 1)
>  
>      if 'actions' in run:
>          env['MOZHARNESS_ACTIONS'] = ' '.join(run['actions'])
>  
>      if 'options' in run:
> -        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options'])
> +        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options']).format(**config.params)

This is pretty general, and applies to a lot of jobs.  I'm a little worried it will get used more widely and become something of a footgun.  My original comment on a similar topic was in https://bugzilla.mozilla.org/show_bug.cgi?id=1306598#c3.

Options:

1. Only substitute `head_repository` and `head_rev` (and document that in the schema in this file).  That at least avoids the temptation to substitute any parameter.
2. Do this in a separate transform, specific to this kind and applied before the build, build_attrs, and build_lints transforms.  Then other users of the mozharness job transforms don't have this functionality available.
3. Get this information from env vars in mozharness, as it's [already provided](https://tools.taskcluster.net/groups/LcldJhU0T-mAD0MiSq44rg/tasks/AYX58EBRRze1xk_0LvAKvw/details)
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review211928

::: taskcluster/taskgraph/actions/release_promotion.py:47
(Diff revision 1)
> +    'push_devedition': {
> +        'target_tasks_method': 'push_devedition',
> +    },
> +    'ship_devedition': {
> +        'target_tasks_method': 'ship_devedition',
>      },

This whole CONFIG looks pretty formulaic at this point.  Is it worth keeping around?
Comment on attachment 8934807 [details]
bug 1423081 - add release updates.

https://reviewboard.mozilla.org/r/205702/#review211960

::: taskcluster/taskgraph/transforms/release_updates.py:5
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +"""
> +Transform the beetmover task into an actual task description.

needs updating?
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211964

::: taskcluster/taskgraph/transforms/task.py:1349
(Diff revision 1)
>          worker_type = task['worker-type'].format(level=level)
>          provisioner_id, worker_type = worker_type.split('/', 1)
> +        project = config.params['project']
>  
>          routes = task.get('routes', [])
> -        scopes = [s.format(level=level) for s in task.get('scopes', [])]
> +        scopes = [s.format(level=level, project=project) for s in task.get('scopes', [])]

This might be OK (we already substitute level here, and for releases project is pretty important).  But it should be documented in the schema above (around line 111).
Comment on attachment 8934813 [details]
bug 1423081 - add shipping-{phase,product} to builds.

https://reviewboard.mozilla.org/r/205714/#review211970

::: taskcluster/ci/build/linux.yml:189
(Diff revision 1)
>          script: "mozharness/scripts/fx_desktop_build.py"
>          secrets: true
>          tooltool-downloads: public
>          need-xvfb: true
>          custom-build-variant-cfg: devedition
> -    run-on-projects: ['mozilla-beta']
> +    run-on-projects: ['mozilla-beta', 'maple']

Intentional to uplift this, or was this just to make it run on maple?  (I don't mind either way, just calling it out in case it wasn't intended)
Comment on attachment 8934823 [details]
bug 1423081 - desktop tc relpro taskcluster docs.

https://reviewboard.mozilla.org/r/205734/#review211972

::: taskcluster/docs/attributes.rst:196
(Diff revision 1)
>  ================
>  For release promotion jobs, this is the product we are shipping.
>  
>  shipping_phase
>  ==============
> -For release promotion jobs, this is the shipping phase (promote, publish, ship).
> +For release promotion jobs, this is the shipping phase (build, promote, push, ship).

Some longer-form, narrative documentation of the release mechanics would be cool.  I admit I don't understand most of the release-related kinds because, I think, I lack a high-level overview.

Maybe that would make sense as a few files in taskcluster/docs/releases, e.g., nightlies.rst, signing.rst, beets-and-balrogs.rst, relpromo.rst.

Definitely not something to do in this bug, just something to think about.

::: taskcluster/docs/kinds.rst:337
(Diff revision 1)
> +--------------------
> +Dummy tasks to handle balrog tasks as dependencies without hitting max-dependencies.
> +
> +post-beetmover-dummy
> +--------------------
> +Dummy tasks to handle beetmover tasks as dependencies without hitting max-dependencies.

Agree that a lot of these are not very helpful (they remind me of documentation that says things like "To create a pivot table, click 'Create Pivot Table', enter the pivot table options, and click 'Save'" -- yeah, I can figure out how to click buttons on my own, but what's a pivot table??).

In particular, even after reading the code I'm not sure how these dummies are used -- if I wanted to depend on some subset of beetmover tasks, how would I do that?

Again, good cleanup-fodder: explain this stuff to the reader as if they were a n00b :)
I didn't mark r+ for these since I don't have the release/relpromo context that Callek does, but what I understand of it (the taskgraph-related bits) looks good, modulo the minor comments above.  It's exciting to see all of this falling into place!
Comment on attachment 8934795 [details]
bug 1423081 - add generate_bz2_blob to schema identifiers.

https://reviewboard.mozilla.org/r/205678/#review212036
Attachment #8934795 - Flags: review?(bugspam.Callek) → review+
Attachment #8934801 - Attachment is obsolete: true
Attachment #8934817 - Attachment is obsolete: true
Attachment #8934817 - Flags: review?(bugspam.Callek)
(In reply to Dustin J. Mitchell [:dustin] from comment #49)
> I didn't mark r+ for these since I don't have the release/relpromo context
> that Callek does, but what I understand of it (the taskgraph-related bits)
> looks good, modulo the minor comments above.  It's exciting to see all of
> this falling into place!

Woo, thank you!

Latest patches pushed, I hope I didn't screw anything up.
Comment on attachment 8934790 [details]
bug 1423081 - add release partials support.

https://reviewboard.mozilla.org/r/205668/#review212124

::: taskcluster/ci/partials-signing/kind.yml:17
(Diff revision 1)
>  kind-dependencies:
>    - partials
> +
> +job-template:
> +  shipping-phase: promote
> +  shipping-product: firefox

Correct. Fixed on maple, should be fixed in the coming patch updates.

::: taskcluster/taskgraph/transforms/partials.py:109
(Diff revision 1)
> +            if 'product' in builds[build]:
> +                partial_info['product'] = builds[build]['product']
> +            if 'previousVersion' in builds[build]:
> +                partial_info['previousVersion'] = builds[build]['previousVersion']
> +            if 'previousBuildNumber' in builds[build]:
> +                partial_info['previousBuildNumber'] = builds[build]['previousBuildNumber']

I think that's for generating partials. For more I might have to send you to Simon, Rail, Ben, Nick? Maybe Mihai.

::: taskcluster/taskgraph/util/partials.py:165
(Diff revision 1)
> +        if "-localtest" in channel:
> +            return channel
> +
> +
> +def populate_release_history(product, branch, maxbuilds=4, maxsearch=10,
> +                             partial_updates=None):

Correct, release history isn't used outside of action tasks currently. Nightlies use partials, but I'm thinking they, too, will become shippable builds that are promoted via action tasks, on some cadence.
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review212128

::: taskcluster/taskgraph/actions/release_promotion.py:47
(Diff revision 1)
> +    'push_devedition': {
> +        'target_tasks_method': 'push_devedition',
> +    },
> +    'ship_devedition': {
> +        'target_tasks_method': 'ship_devedition',
>      },

Good point :) We still allow for defining `do_not_optimize` and `rebuild_kinds` in the config, but haven't needed to yet. I'm not 100% sure we won't need those as we hit real world issues. I'm leaning towards keeping this until we prove we don't need those two config items, and then we can tear it out. Does that work?
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review212148
Comment on attachment 8934792 [details]
bug 1423081 - source readme.

https://reviewboard.mozilla.org/r/205672/#review212132

::: taskcluster/taskgraph/transforms/job/mozharness.py:146
(Diff revision 1)
>  
>      if 'actions' in run:
>          env['MOZHARNESS_ACTIONS'] = ' '.join(run['actions'])
>  
>      if 'options' in run:
> -        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options'])
> +        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options']).format(**config.params)

I'm leaning towards 1 or 3. Keeping this open for now.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review212136

::: taskcluster/taskgraph/transforms/task.py:1490
(Diff revision 1)
> -                if v is None:
> -                    continue
> -                elif isinstance(v, list):
> -                    v = {'ids': v}
> -                    if 'completed' == k:
> -                        v.update({
> +                    resolve_keyed_by(
> +                        notification,
> +                        notification_option,
> +                        'notifications',
> +                        project=config.params['project'],
> +                    )

A number of these issues are dealt with in the next round of patch updates.
(In reply to Justin Wood (:Callek) from comment #39)
> Comment on attachment 8934801 [details]
> bug 1423081 - add devedition+source to the gecko_v2_whitelist.
> 
> https://reviewboard.mozilla.org/r/205690/#review211652
> 
> ::: commit-message-88265:1
> (Diff revision 1)
> > +bug 1423081 - add devedition+source to the gecko_v2_whitelist. r=callek
> 
> This patch makes me wonder if "{}-devedition" can be removed in favor of the
> "{}-devedition-opt" entries. (we'll probably just drop the whitelist once bb
> is dead, so not a requirement)

Yeah, I don't understand the whitelist, so I just added entries til it stopped complaining. Dropping it once bb is dead sgtm.
(In reply to Dustin J. Mitchell [:dustin] from comment #47)
> Comment on attachment 8934813 [details]
> bug 1423081 - add shipping-{phase,product} to builds.
> 
> https://reviewboard.mozilla.org/r/205714/#review211970
> 
> ::: taskcluster/ci/build/linux.yml:189
> (Diff revision 1)
> >          script: "mozharness/scripts/fx_desktop_build.py"
> >          secrets: true
> >          tooltool-downloads: public
> >          need-xvfb: true
> >          custom-build-variant-cfg: devedition
> > -    run-on-projects: ['mozilla-beta']
> > +    run-on-projects: ['mozilla-beta', 'maple']
> 
> Intentional to uplift this, or was this just to make it run on maple?  (I
> don't mind either way, just calling it out in case it wasn't intended)

The latter. I don't have strong feelings either way, but if we land maple support we should land all of it intentionally.
Removed from my patch queue; I'll submit that change the next round.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review212184
Comment on attachment 8934807 [details]
bug 1423081 - add release updates.

https://reviewboard.mozilla.org/r/205702/#review211960

> needs updating?

Updated on maple, will update review queue next round.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> Dustin has once scolded me for doing this ({project}) replacement in transforms. It is why I have a big warning in l10n.py transforms [1]
> 
> Can we do some sort of comment here that expanding scope on project is only for buildbot-bridge or something. (or drop it entirely if we don't actually need it)
> 
> [1] - https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/l10n.py#344

Addressed by updating the schema as Dustin suggested.

> Notification stuff is a bit hard to reason about here, and increases the size of this function and with it cognitive load...
> 
> I'd love if we either made a seperate transform in this file, or a seperate transform file itself to support notification stuff.
> 
> This can be relegated to followup.

This was cleaned up significantly in the latest push; let me know if you still want this in a separate file.
Comment on attachment 8934823 [details]
bug 1423081 - desktop tc relpro taskcluster docs.

https://reviewboard.mozilla.org/r/205734/#review211656

> nit: would love a longer description of what each phase is, so that future work has a definition of what belongs in each phase. 
> 
> We can postpone that for "cleanup" though.

Addressed on maple; will update the review queue...

> s/release source This/release source. This/

Fixed on maple..

> I think many of these release-* kind descriptions are a bit under-verbose, e.g. "What is binary transparency, what sort of certificate..." and "What is a snap"  --> "Task that generates an installer using Ubuntu's Snap format"
> 
> This is more cleanup fodder though

Added a Trello card.

> nit (x2) "Dummy tasks to consolidate .... dependencies used in order to avoid taskcluster limits on number of dependencies per task" or some such.

Fixed on maple...
(In reply to Justin Wood (:Callek) from comment #37)
> ::: taskcluster/taskgraph/transforms/partials.py:109
> (Diff revision 1)
> > +            if 'product' in builds[build]:
> > +                partial_info['product'] = builds[build]['product']
> > +            if 'previousVersion' in builds[build]:
> > +                partial_info['previousVersion'] = builds[build]['previousVersion']
> > +            if 'previousBuildNumber' in builds[build]:
> > +                partial_info['previousBuildNumber'] = builds[build]['previousBuildNumber']
> 
> I'm not sure I understand what these previous* things are used for. I could
> use some help there...  (since they are passed as data into the generation
> of partials for funsize)

These are the "from" versions. Maybe this pseudo code would explain it better:

to_version = '60.0'
to_build_number = 1

partials = [
    {'previousVersion': '59.0', previousBuildNumber: 1},
    {'previousVersion': '58.0', previousBuildNumber: 1},
]

for previousVersion, previousBuildNumber in partials:
    print "will generate ${previousVersion}-${previousBuildNumber} to ${to_version}-${to_build_number} partials"

Should output

will generate 59.0-1 to 60.0.1 partials
will generate 58.0-1 to 60.0.1 partials

> ::: taskcluster/taskgraph/util/partials.py:165
> (Diff revision 1)
> > +        if "-localtest" in channel:
> > +            return channel
> > +
> > +
> > +def populate_release_history(product, branch, maxbuilds=4, maxsearch=10,
> > +                             partial_updates=None):
> 
> I don't see partial_updates being utilized from the decision graph itself.
> 
> Is it intentional that we don't seem to have any way to fill that in other
> than via action tasks?

partials are set in ship-it, so they are not the part of the decision task; they are passed via input to the first action task.
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review212370

::: taskcluster/taskgraph/actions/release_promotion.py:292
(Diff revisions 1 - 2)
> +    # that didn't exist in the first full_task_graph, so combining them is
> +    # important.
> +    combined_full_task_graph = {}
> +    for graph_id in previous_graph_ids:
> +        full_task_graph = get_artifact(graph_id, "public/full-task-graph.json")
> +        combined_full_task_graph.update(full_task_graph)

I have a conceptual wonder about what happens if there is a conflict in this .update() (I know it replaces it, but what conceptually do we want to happen if the contents don't ===)

I don't have a strong concern here, so feel free to say that its not really a worry and resolve.
Attachment #8934791 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934790 [details]
bug 1423081 - add release partials support.

https://reviewboard.mozilla.org/r/205668/#review212376
Attachment #8934790 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934792 [details]
bug 1423081 - source readme.

https://reviewboard.mozilla.org/r/205672/#review212426

::: taskcluster/ci/release-source/kind.yml:45
(Diff revision 2)
> +         emails:
> +            by-project:
> +               mozilla-beta: ["release-automation-notifications@mozilla.com"]
> +               mozilla-release: ["release-automation-notifications@mozilla.com"]
> +               try: ["{task_def[metadata][owner]}"]
> +               maple: ["release+tcstaging@mozilla.com"]

possibly don't need maple here, but no big deal if it stays. (x2)

::: taskcluster/ci/release-source/source.yml:17
(Diff revision 2)
> +    run:
> +        using: mozharness
> +        actions: [build]
> +        config:
> +            - builds/releng_sub_linux_configs/64_source.py
> +        options: ["repo={head_repository}", "revision={head_rev}"]

[x3] We could pull this from the environment rather than transform the values... (I think this was suggested elsewhere)

::: taskcluster/taskgraph/transforms/beetmover.py:198
(Diff revision 2)
>      'win32-nightly-l10n': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_L10N,
>      'win64-nightly-l10n': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_L10N,
>  }
>  # Until bug 1331141 is fixed, if you are adding any new artifacts here that
>  # need to be transfered to S3, please be aware you also need to follow-up
>  # with a beetmover patch in https://github.com/mozilla-releng/beetmoverscript/.

In this case, I'm assuming beetmover is patched

::: taskcluster/taskgraph/transforms/beetmover_source.py:45
(Diff revision 2)
> +                )
> +            )
> +        upstream_artifacts = job['worker']['upstream-artifacts']
> +        for artifact in upstream_artifacts:
> +            if artifact['taskType'] == 'build':
> +                artifact['paths'].append(u'public/build/balrog_props.json')

this might be safer as `artifact['paths'] = [...]` since we're changing the build target above. And in the beetmover transform itself an '# XXX Unused, see comment in `beetmover_source` transform or some such'

Merely a nit, skippable per author discretion

::: taskcluster/taskgraph/transforms/job/mozharness.py:146
(Diff revision 2)
>  
>      if 'actions' in run:
>          env['MOZHARNESS_ACTIONS'] = ' '.join(run['actions'])
>  
>      if 'options' in run:
> -        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options'])
> +        env['MOZHARNESS_OPTIONS'] = ' '.join(run['options']).format(**config.params)

if we don't change to pulling by environ, can we at least just pass the specific params we know we care about here (or use a seperate, earlier, transform)

::: taskcluster/taskgraph/util/signed_artifacts.py:14
(Diff revision 2)
>  
>  
>  def generate_specifications_of_artifacts_to_sign(
> -    build_platform, is_nightly=False, keep_locale_template=True
> +    build_platform, is_nightly=False, keep_locale_template=True, kind=None
>  ):
> -    if 'android' in build_platform:
> +    if kind == 'release-source-signing':

I'm not really a fan of having to switch this on kind name here, *but* I don't have a better suggestion offhand, so unless you do, we'll keep it.
Attachment #8934792 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934793 [details]
bug 1423081 - target_tasks_methods updates.

https://reviewboard.mozilla.org/r/205674/#review212454

::: taskcluster/taskgraph/target_tasks.py:309
(Diff revision 2)
> -@_target_task('maple_desktop_promotion')
> -@_target_task('mozilla-beta_desktop_promotion')
> +@_target_task('promote_firefox')
> +def target_tasks_promote_firefox(full_task_graph, parameters, graph_config):
> -@_target_task('mozilla-release_desktop_promotion')
> -def target_tasks_mozilla_beta_desktop_promotion(full_task_graph, parameters, graph_config):
>      """Select the superset of tasks required to promote a beta or release build
> -    of desktop. This should include all non-android mozilla_beta tasks, plus
> +    of firefox. This should include all non-android mozilla_beta tasks, plus

nit "mozilla_beta" in doc...

::: taskcluster/taskgraph/target_tasks.py:312
(Diff revision 2)
> -def target_tasks_mozilla_beta_desktop_promotion(full_task_graph, parameters, graph_config):
>      """Select the superset of tasks required to promote a beta or release build
> -    of desktop. This should include all non-android mozilla_beta tasks, plus
> +    of firefox. This should include all non-android mozilla_beta tasks, plus
>      l10n, beetmover, balrog, etc."""
>  
>      beta_tasks = [l for l, t in full_task_graph.tasks.iteritems() if

lets genericize the naming here, there is the phrase "beta" and "beta_tasks" type comments/variables a bunch here.

::: taskcluster/taskgraph/target_tasks.py:350
(Diff revision 2)
> -        # TODO: tc update verify
> -        # TODO: beetmover push-to-candidates
> -        # TODO: binary transparency
>          # TODO: bouncer sub
> -        # TODO: snap
>          # TODO: recompression tasks

Nomenclature question, what is a "recompression task"

::: taskcluster/taskgraph/target_tasks.py:409
(Diff revision 2)
> +def target_tasks_promote_devedition(full_task_graph, parameters, graph_config):
> +    """Select the superset of tasks required to promote a beta or release build
> +    of devedition. This should include all non-android mozilla_beta tasks, plus
> +    l10n, beetmover, balrog, etc."""
> +
> +    beta_tasks = [l for l, t in full_task_graph.tasks.iteritems() if

same comment about "beta"
Attachment #8934793 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934794 [details]
bug 1423081 - inherit shipping-product from upstream.

https://reviewboard.mozilla.org/r/205676/#review212458

::: commit-message-17b68:1
(Diff revision 2)
> +bug 1423081 - inherit shipping-{phase,product} from upstream. r=callek

nit: I don't see anything inheriting shipping-phase from upstream here, just product.
Attachment #8934794 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934796 [details]
bug 1423081 - add firefox and devedition relpro support to scriptworker.py

https://reviewboard.mozilla.org/r/205680/#review212460

::: taskcluster/taskgraph/util/scriptworker.py:457
(Diff revision 2)
> +        partial_updates = json.loads(partial_updates)
> +        release_config['partial_versions'] = ', '.join([
> +            '{}build{}'.format(v, info['buildNumber'])
> +            for v, info in partial_updates.items()
> +        ])
> +        if release_config['partial_versions'] == "{}":

where would we get "{}" from for this?
Attachment #8934796 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934797 [details]
bug 1423081 - add release balrog support.

https://reviewboard.mozilla.org/r/205682/#review212466

::: taskcluster/ci/balrog/kind.yml:22
(Diff revision 2)
>  only-for-attributes:
>     - nightly
>     - signed
> +
> +job-template:
> +   notifications:

I'm a bit worried about someone accidentally causing balrog notifications on central, but it won't as written so no need to redesign right now
Attachment #8934797 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review212370

> I have a conceptual wonder about what happens if there is a conflict in this .update() (I know it replaces it, but what conceptually do we want to happen if the contents don't ===)
> 
> I don't have a strong concern here, so feel free to say that its not really a worry and resolve.

We explicitly want the latest graph (rightmost) to take precedence in the case of dup/conflicting keys. Would a comment help here?
Comment on attachment 8934798 [details]
bug 1423081 - add desktop release beetmover support.

https://reviewboard.mozilla.org/r/205684/#review212506

::: taskcluster/taskgraph/transforms/beetmover_repackage.py:432
(Diff revision 2)
>          extra = list()
>  
>          artifact_map = get_partials_artifact_map(
>              config.params.get('release_history'), balrog_platform, locale)
>          for artifact in artifact_map:
> -            extra.append({
> +            _ = {

nit: var name should be something other than `_`  (I'm not a fan of _ for anything expected to be used...)
Attachment #8934798 - Flags: review?(bugspam.Callek) → review+
(In reply to Aki Sasaki [:aki] from comment #106)
> Comment on attachment 8934791 [details]
> bug 1423081 - add firefox + devedition relpro.
> 
> https://reviewboard.mozilla.org/r/205670/#review212370
> 
> > I have a conceptual wonder about what happens if there is a conflict in this .update() (I know it replaces it, but what conceptually do we want to happen if the contents don't ===)
> > 
> > I don't have a strong concern here, so feel free to say that its not really a worry and resolve.
> 
> We explicitly want the latest graph (rightmost) to take precedence in the
> case of dup/conflicting keys. Would a comment help here?

I do think it would for future reading of this, yea.
Comment on attachment 8934799 [details]
bug 1423081 - desktop release update verify support.

https://reviewboard.mozilla.org/r/205686/#review212526

::: taskcluster/ci/release-buildbot-update-verify/kind.yml:9
(Diff revision 2)
> +
> +loader: taskgraph.loader.transform:loader
> +
> +kind-dependencies:
> +   # TODO: this makes us depend on beetmover/balrog for all platforms
> +   # does it matter?

make sure this question in the TODO is figured out before landing ;-)

::: taskcluster/ci/release-buildbot-update-verify/kind.yml:128
(Diff revision 2)
> +                  jamun: "beta-firefox-win32.cfg"
> +                  maple: "beta-firefox-win32.cfg"
> +                  mozilla-beta: "beta-firefox-win32.cfg"
> +                  mozilla-release: "release-firefox-win32.cfg"
> +                  mozilla-esr52: "esr-firefox-win32.cfg"
> +                  default: "none"

can use:  null | Null | NULL | ~

This will eval out into python `None` as a possible improvement in code/readability here

::: taskcluster/ci/release-final-verify/kind.yml:9
(Diff revision 2)
> +
> +loader: taskgraph.loader.transform:loader
> +
> +kind-dependencies:
> +   # release-uptake-monitoring will get removed as a dep
> +   # for secondary channel final verify

since final_verify has a seperate kind for secondary, drop this comment ;-)

::: taskcluster/ci/release-final-verify/kind.yml:24
(Diff revision 2)
> +   run-on-projects: []  # to make sure this never runs as part of CI
> +   worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> +   worker:
> +      implementation: docker-worker
> +      os: linux
> +      docker-image: mozillareleases/python-test-runner@sha256:0729c2e6e7bc0d6a4cbccb2e66a78e1d8e8cbb5e44105d56e3c9c610230ebd69

cleanup: lets get this docker image in tree ;-)

::: taskcluster/ci/release-secondary-final-verify/kind.yml:9
(Diff revision 2)
> +
> +loader: taskgraph.loader.transform:loader
> +
> +kind-dependencies:
> +    # TODO: this makes us depend on beetmover/balrog for all platforms
> +    # does it matter?

solve this todo before landing too.

::: taskcluster/ci/release-update-verify/kind.yml:3
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

is there a reason we can't use `jobs-from:` and set `job-defaults` appropriately for release-update-verify and the buildbot variant.

To avoid an extra kind here, or do they need drastically different transforms/implementations that it doesn't work out like I think?

::: taskcluster/taskgraph/transforms/buildbot_update_verify.py:7
(Diff revision 2)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +"""
> +Transform the beetmover task into an actual task description.
> +"""
> +

The majority of this transform could be done as a job/* (as in run: using: buildbot_update_verify) if its easier/clearer. (and if it eases merging both UV kinds together)

::: taskcluster/taskgraph/transforms/buildbot_update_verify.py:38
(Diff revision 2)
> +        for this_chunk in range(1, total_chunks+1):
> +            chunked = deepcopy(task)
> +            chunked["scopes"] = [
> +                "project:releng:buildbot-bridge:builder-name:{}".format(
> +                    # In scopes, "branch" is called "project"
> +                    buildername.replace("branch", "project")

I'd probably suggest buildername.format(branch="{project}") merely because it prevents something like a job named `branching_sanity` becoming `projecting_sanity`. But buildbot is dying so no big deal

::: taskcluster/taskgraph/transforms/update_verify.py:40
(Diff revision 2)
> +            if not chunked["worker"].get("env"):
> +                chunked["worker"]["env"] = {}
> +            chunked["worker"]["command"] = [
> +                "/bin/bash",
> +                "-c",
> +                "hg clone $BUILD_TOOLS_REPO tools && cd tools && " +

I'd love if we had a better cloning story here (retries/robustcheckout/etc) but I think that should all be followup fodder
Attachment #8934799 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934808 [details]
bug 1423081 - add shipping-{phase,product} to repackage.

https://reviewboard.mozilla.org/r/205704/#review212660
Attachment #8934808 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934810 [details]
bug 1423081 - shipping-{phase,product} in signing.

https://reviewboard.mozilla.org/r/205708/#review212662
Attachment #8934810 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934813 [details]
bug 1423081 - add shipping-{phase,product} to builds.

https://reviewboard.mozilla.org/r/205714/#review212664

A few designations of maple in here, but no worry if they land.
Attachment #8934813 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934800 [details]
bug 1423081 - desktop release update/final verify support.

https://reviewboard.mozilla.org/r/205688/#review212696

::: taskcluster/ci/release-secondary-final-verify/kind.yml:9
(Diff revision 2)
>  
>  loader: taskgraph.loader.transform:loader
>  
>  kind-dependencies:
> -    # TODO: this makes us depend on beetmover/balrog for all platforms
> +   # TODO: this makes us depend on beetmover/balrog for all platforms
> -    # does it matter?
> +   # does it matter?

Same comment about figuring out this todo
Attachment #8934800 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934803 [details]
bug 1423081 - devedition l10n support.

https://reviewboard.mozilla.org/r/205694/#review212698

::: taskcluster/ci/nightly-l10n/kind.yml:89
(Diff revision 2)
>              default: firefox
> +            linux32-devedition-nightly: devedition
> +            linux64-devedition-nightly: devedition
> +            macos64-devedition-nightly: devedition
> +            win32-devedition-nightly: devedition
> +            win64-devedition-nightly: devedition

This is fine, but you could do .*-devedition-.* to specify more concisely
Attachment #8934803 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934805 [details]
bug 1423081 - add dynamic dep_tasks based on shipping-phase.

https://reviewboard.mozilla.org/r/205698/#review212700

::: taskcluster/taskgraph/transforms/release_deps.py:58
(Diff revision 2)
>                  # Skip old-id
>                  if 'old-id' in dep_task.label:
>                      continue
> +            # We can only depend on tasks within the same phase
> +            if dep_task.attributes.get('shipping_phase') != phase:
> +                continue

I didn't know this was a limitation, does that mean we can't schedule next phase if previous phase isn't competely done?

::: taskcluster/taskgraph/transforms/release_deps.py:65
(Diff revision 2)
>              if _get_product(dep_task.task) == product:
>                  dependencies[dep_task.label] = dep_task.label
> +            # this is dumb, but i don't know how to get beetmover to set
> +            # shipping-product in a way that _get_product recognizes
> +            if dep_task.attributes.get('shipping_product') == product:
> +                dependencies[dep_task.label] = dep_task.label

I bet this can be fixed, or at least setup to only be in beetmover transforms. At the least can someone explain the reasoning better?
Attachment #8934805 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934809 [details]
bug 1423081 - reverse chunk deps.

https://reviewboard.mozilla.org/r/205706/#review212702

::: commit-message-969c1:5
(Diff revision 2)
> +bug 1423081 - reverse chunk deps. r=callek
> +
> +This allows us to funnel large numbers of tasks down to avoid hitting
> +MAX_DEPENDENCIES. I avoided using a morph here because we might break
> +certain cot assumptions.

This is certainly the type of task well suited for a morph. We should as a followup figure out how to get cot to let us do it that way.

::: taskcluster/ci/post-balrog-dummy/kind.yml:32
(Diff revision 2)
> +         command:
> +            - /bin/bash
> +            - -c
> +            - echo "Dummy task"
> +      treeherder:
> +         symbol: Rel(BD)

I'd be ok with the tasks in this commit not even reporting to treeherder.

I suspect that we will end up using https://github.com/taskcluster/taskcluster-rfcs/issues/75 here at some point

::: taskcluster/taskgraph/transforms/reverse_chunk_deps.py:37
(Diff revision 2)
> +    return job
> +
> +
> +@transforms.add
> +def add_dependencies(config, jobs):
> +    for job in release_deps.add_dependencies(config, jobs):

This is an odd construct, iterating through another transform to handle our transform. 

This might need some explanatory text
Attachment #8934809 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934814 [details]
bug 1423081 - add new docker image tasks.

https://reviewboard.mozilla.org/r/205716/#review212704
Attachment #8934814 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934807 [details]
bug 1423081 - add release updates.

https://reviewboard.mozilla.org/r/205702/#review212706

::: taskcluster/taskgraph/transforms/release_updates.py:6
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +"""
> +Transform the beetmover task into an actual task description.
> +"""

Beetmover?
Attachment #8934807 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934820 [details]
bug 1423081 - release updates builder support.

https://reviewboard.mozilla.org/r/205728/#review212708

::: taskcluster/ci/release-updates-builder/kind.yml:84
(Diff revision 2)
> +      worker:
> +         properties:
> +            # fixme if this is actually used, kill otherwise
> +            # revision: ?
> +            # fixme if this is actually used - kill me otherwise
> +            # script_repo_revision: ?

X2 places, resolve fixmes before landing (it's ok to reference a follow-up bug to resolve)
Attachment #8934820 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934804 [details]
bug 1423081 - partner_repack support.

https://reviewboard.mozilla.org/r/205696/#review212828

::: taskcluster/taskgraph/transforms/partner_repack.py:18
(Diff revision 2)
> +
> +transforms = TransformSequence()
> +
> +
> +@transforms.add
> +def add_command(config, tasks):

This transform name doesn't seem to match what its doing. (It's not adding any command)
Attachment #8934804 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934806 [details]
bug 1423081 - snap support.

https://reviewboard.mozilla.org/r/205700/#review212830
Attachment #8934806 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934815 [details]
bug 1423081 - binary transparency kind.

https://reviewboard.mozilla.org/r/205718/#review212832

Clearing review, since it feels like there is more to do on this one.

::: taskcluster/ci/release-binary-transparency/kind.yml:13
(Diff revision 2)
> +   # - taskgraph.transforms.release_deps:transforms
> +   - taskgraph.transforms.task:transforms
> +
> +# TODO: once release checksums is done
> +# kind-dependencies:
> +#    - release-checksums

This probably shouldn't land until the commented-out bits above this are solved (the transform and the kind-dependency stuff)
Attachment #8934815 - Flags: review?(bugspam.Callek)
Comment on attachment 8934816 [details]
bug 1423081 - desktop bouncer sub+aliases.

https://reviewboard.mozilla.org/r/205720/#review212834
Attachment #8934816 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934818 [details]
bug 1423081 - desktop release-mark-as-shipped support.

https://reviewboard.mozilla.org/r/205724/#review212836
Attachment #8934818 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934819 [details]
bug 1423081 - release-notify email updates.

https://reviewboard.mozilla.org/r/205726/#review212838
Attachment #8934819 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934821 [details]
bug 1423081 - desktop uptake monitoring support.

https://reviewboard.mozilla.org/r/205730/#review212840

::: taskcluster/ci/release-uptake-monitoring/kind.yml:84
(Diff revision 2)
> +      run:
> +         product: fennec
> +         buildername: release-{branch}-fennec_uptake_monitoring
> +      worker:
> +         properties:
> +            # TODO: Calculate "platforms" dynamically

can we point # TODO at a bug # instead?
Attachment #8934821 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934822 [details]
bug 1423081 - desktop release-version-bump support.

https://reviewboard.mozilla.org/r/205732/#review212842
Attachment #8934822 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934824 [details]
bug 1423081 - desktop tc relpro dev mozharness configs.

https://reviewboard.mozilla.org/r/205736/#review212844

I'm just rubberstamping this one, its only dev configs. (let me know seperately if you need a thorough review here)
Attachment #8934824 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8935585 [details]
bug 1423081 - add checksums builder.

https://reviewboard.mozilla.org/r/206466/#review212846
Attachment #8935585 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934792 [details]
bug 1423081 - source readme.

https://reviewboard.mozilla.org/r/205672/#review211942

> This is pretty general, and applies to a lot of jobs.  I'm a little worried it will get used more widely and become something of a footgun.  My original comment on a similar topic was in https://bugzilla.mozilla.org/show_bug.cgi?id=1306598#c3.
> 
> Options:
> 
> 1. Only substitute `head_repository` and `head_rev` (and document that in the schema in this file).  That at least avoids the temptation to substitute any parameter.
> 2. Do this in a separate transform, specific to this kind and applied before the build, build_attrs, and build_lints transforms.  Then other users of the mozharness job transforms don't have this functionality available.
> 3. Get this information from env vars in mozharness, as it's [already provided](https://tools.taskcluster.net/groups/LcldJhU0T-mAD0MiSq44rg/tasks/AYX58EBRRze1xk_0LvAKvw/details)

Fixed on maple with (3); will update the review queue during the next round.
Comment on attachment 8934791 [details]
bug 1423081 - add firefox + devedition relpro.

https://reviewboard.mozilla.org/r/205670/#review212370

> We explicitly want the latest graph (rightmost) to take precedence in the case of dup/conflicting keys. Would a comment help here?

Added another comment on maple.
Comment on attachment 8934792 [details]
bug 1423081 - source readme.

https://reviewboard.mozilla.org/r/205672/#review212426

> [x3] We could pull this from the environment rather than transform the values... (I think this was suggested elsewhere)

Yeah, fixed in subsequent pushes to maple.

> this might be safer as `artifact['paths'] = [...]` since we're changing the build target above. And in the beetmover transform itself an '# XXX Unused, see comment in `beetmover_source` transform or some such'
> 
> Merely a nit, skippable per author discretion

We're tracking this in both https://trello.com/c/eImDIb4X/188-solve-the-beetmover-source-hack-and-address-it-via-mozharness-instead-of-altering-the-deps-upstreamartifacts and https://trello.com/c/fwJruFlS/186-rewrite-the-beetmover-dependency-system-its-dirty-and-not-flexible-enough-to-meet-our-needs . Both are in cleanup, so I'll drop this for now.

> if we don't change to pulling by environ, can we at least just pass the specific params we know we care about here (or use a seperate, earlier, transform)

Already addressed in environ.

> I'm not really a fan of having to switch this on kind name here, *but* I don't have a better suggestion offhand, so unless you do, we'll keep it.

Yeah, we want to make artifacts and beetmover all data/yaml driven. In cleanup.
Comment on attachment 8934793 [details]
bug 1423081 - target_tasks_methods updates.

https://reviewboard.mozilla.org/r/205674/#review212454

> nit "mozilla_beta" in doc...

That's actually accurate - mozilla_beta_tasks is pretty much shared with mozilla_release. Amended to `mozilla_{beta,release}` for clarity.

> lets genericize the naming here, there is the phrase "beta" and "beta_tasks" type comments/variables a bunch here.

Changed to beta_release_tasks.

> Nomenclature question, what is a "recompression task"

bz2/lzma
Comment on attachment 8934794 [details]
bug 1423081 - inherit shipping-product from upstream.

https://reviewboard.mozilla.org/r/205676/#review212458

> nit: I don't see anything inheriting shipping-phase from upstream here, just product.

Amended in my local review queue; pushing that with the other changes in the next day or two.
Comment on attachment 8934796 [details]
bug 1423081 - add firefox and devedition relpro support to scriptworker.py

https://reviewboard.mozilla.org/r/205680/#review212460

> where would we get "{}" from for this?

The line above: I imagine an empty PARTIAL_UPDATES env var. We check for "" but not "{}" or "[]" above, probably to avoid a json ValueError, but we don't check for an otherwise empty value.
Comment on attachment 8934798 [details]
bug 1423081 - add desktop release beetmover support.

https://reviewboard.mozilla.org/r/205684/#review212506

> nit: var name should be something other than `_`  (I'm not a fan of _ for anything expected to be used...)

+1. Renamed to `artifact_extra`
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review212870
Attachment #8934811 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934804 [details]
bug 1423081 - partner_repack support.

https://reviewboard.mozilla.org/r/205696/#review212828

> This transform name doesn't seem to match what its doing. (It's not adding any command)

renamed to `resolve_properties`.
Comment on attachment 8934805 [details]
bug 1423081 - add dynamic dep_tasks based on shipping-phase.

https://reviewboard.mozilla.org/r/205698/#review212700

> I didn't know this was a limitation, does that mean we can't schedule next phase if previous phase isn't competely done?

This isn't a limitation; fixed on maple.

> I bet this can be fixed, or at least setup to only be in beetmover transforms. At the least can someone explain the reasoning better?

This is an old comment, but this highlighted another bug. Both addressed on maple.
Comment on attachment 8934807 [details]
bug 1423081 - add release updates.

https://reviewboard.mozilla.org/r/205702/#review212706

> Beetmover?

Addressed on maple.
Comment on attachment 8934815 [details]
bug 1423081 - binary transparency kind.

https://reviewboard.mozilla.org/r/205718/#review212832

> This probably shouldn't land until the commented-out bits above this are solved (the transform and the kind-dependency stuff)

Just some dep issues. Fixed on maple.
Comment on attachment 8934821 [details]
bug 1423081 - desktop uptake monitoring support.

https://reviewboard.mozilla.org/r/205730/#review212840

> can we point # TODO at a bug # instead?

Already a trello card; I can remove the line. https://trello.com/c/tcP7IaoT/199-compute-uptake-monitoring-platforms-dynamically
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> nit: update the comment to say that an array of exit status codes is ok.

Fixed on maple.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> what cases will we not have a job-name and still be valid?

Removed the .get() on maple.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> why don't we want to verify here anymore?

Release indexes don't use job-name at all, so verifying it seems redundant / overkill. Once we removed the requirement for release indexes to have a job-name, we added a bunch of tasks without a job-name, so if we want to verify (even though the index doesn't use it) that'll require a non-trivial amount of cleanup.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> I'd rather we have a sanity check/exception here if task['shipping-product'] !== attributes['shipping_product'] when set.
> 
> Since doing otherwise would cause confusion in the job-defaults and such. (It may have caused me some confusion in some already-looked at reviews).

Added a sanity check on maple. I'd rather this be nicer, but this should help for now.
Comment on attachment 8934799 [details]
bug 1423081 - desktop release update verify support.

https://reviewboard.mozilla.org/r/205686/#review212526

> can use:  null | Null | NULL | ~
> 
> This will eval out into python `None` as a possible improvement in code/readability here

Probably doesn't matter (either should cause update verify to fail), but that seems more correct.

> since final_verify has a seperate kind for secondary, drop this comment ;-)

+1

> cleanup: lets get this docker image in tree ;-)

Yeah, this is for future. We might just be able to use (and rename) the update-verify docker image as-is, or tweak it slightly.

> solve this todo before landing too.

I don't think we should block on solving this, but maybe we can decide if this less-than-ideal dependency is worth adding to the cleanup list.

> is there a reason we can't use `jobs-from:` and set `job-defaults` appropriately for release-update-verify and the buildbot variant.
> 
> To avoid an extra kind here, or do they need drastically different transforms/implementations that it doesn't work out like I think?

I think they're pretty drastically different, because of all the buildbot bridge parts. If the buildbot one was going to be longer lived it might be worth trying to unify them, but I think it's probably going to die in January anyways. I don't think the cost/benefit of overhauling this is worthwhile at this point.

> The majority of this transform could be done as a job/* (as in run: using: buildbot_update_verify) if its easier/clearer. (and if it eases merging both UV kinds together)

This sounds correct (I'll defer to you on that), but like the above I'm not sure it's worth doing at this point. The current thing works, and is going away in the very near term.

> I'd love if we had a better cloning story here (retries/robustcheckout/etc) but I think that should all be followup fodder

Yep, that's for cleanup. It might go away completely when we kill patcher configs, too.
Comment on attachment 8934820 [details]
bug 1423081 - release updates builder support.

https://reviewboard.mozilla.org/r/205728/#review212708

> X2 places, resolve fixmes before landing (it's ok to reference a follow-up bug to resolve)

These can go, the updates builder is working without them!
Comment on attachment 8934799 [details]
bug 1423081 - desktop release update verify support.

https://reviewboard.mozilla.org/r/205686/#review212526

> make sure this question in the TODO is figured out before landing ;-)

This is probably something for cleanup. Or maybe we can decide now if we want to bother addressing it.
Comment on attachment 8934800 [details]
bug 1423081 - desktop release update/final verify support.

https://reviewboard.mozilla.org/r/205688/#review212696

> Same comment about figuring out this todo

This is probably something for cleanup. Or maybe we can decide now if we want to bother addressing it.
Comment on attachment 8934799 [details]
bug 1423081 - desktop release update verify support.

https://reviewboard.mozilla.org/r/205686/#review212526

> I don't think we should block on solving this, but maybe we can decide if this less-than-ideal dependency is worth adding to the cleanup list.

I was mostly meaning solve the "does it matter" question in some way. If its a "not really" then meh, if its a "it would be better in cleanup" then cleanup it goes, if it matters stronger than that then we block. I just don't want the question itself. lingering
Comment on attachment 8934799 [details]
bug 1423081 - desktop release update verify support.

https://reviewboard.mozilla.org/r/205686/#review212526

> This is probably something for cleanup. Or maybe we can decide now if we want to bother addressing it.

Added https://trello.com/c/C4jHhhqG/231-revisit-buildbot-update-verify-deps ; going to kill the TODO comment.

> Probably doesn't matter (either should cause update verify to fail), but that seems more correct.

Using null breaks graph generation, since it requires a string. Going to wontfix this.

> +1

Done on maple.

> I was mostly meaning solve the "does it matter" question in some way. If its a "not really" then meh, if its a "it would be better in cleanup" then cleanup it goes, if it matters stronger than that then we block. I just don't want the question itself. lingering

Added to https://trello.com/c/C4jHhhqG/231-revisit-buildbot-update-verify-release-final-verify-deps .

> I think they're pretty drastically different, because of all the buildbot bridge parts. If the buildbot one was going to be longer lived it might be worth trying to unify them, but I think it's probably going to die in January anyways. I don't think the cost/benefit of overhauling this is worthwhile at this point.

Dropping due to this comment.
Comment on attachment 8934800 [details]
bug 1423081 - desktop release update/final verify support.

https://reviewboard.mozilla.org/r/205688/#review212696

> This is probably something for cleanup. Or maybe we can decide now if we want to bother addressing it.

Added to https://trello.com/c/C4jHhhqG/231-revisit-buildbot-update-verify-release-final-verify-release-secondary-final-verify-deps .
Comment on attachment 8934820 [details]
bug 1423081 - release updates builder support.

https://reviewboard.mozilla.org/r/205728/#review212708

> These can go, the updates builder is working without them!

Fixed on maple.
Comment on attachment 8934811 [details]
bug 1423081 - desktop release support in transforms/task.py

https://reviewboard.mozilla.org/r/205710/#review211848

> None of these other entries in the worker are utilized from any passed down tasks in maple.
> 
> Are they required to be configurable or should we hardcode in the payload builder below to cut out complexity for now?

Removed complexity for now.

> As mentioned above this doesn't actually seem used by entries here.
> 
> Of note of course, is that the dict of the job is passed by reference (not copy) [unless something changed outside of my memory] and that resolve_keyed_by replaces values.
> 
> So if you're not doing a .deepcopy() you run the risk of having the return of this resolve not being valid against any other possible returns from the same taskgraph. (as in, if you resolve by product, whichever product gets run first will be sticky and you won't get the resolve for next product).
> 
> Though as I said, nothing in the current maple branch uses this param, so you get metadata.owner anyway instead ;-)

Resolved on maple with hardcodes, esp since binary-transparency is a dummy worker task atm.
Attachment #8934815 - Flags: review?(bugspam.Callek)
Attachment #8934799 - Attachment is obsolete: true
Comment on attachment 8936668 [details]
bug 1423081 - Add balrog publishing task.

https://reviewboard.mozilla.org/r/207408/#review213286

::: taskcluster/ci/release-balrog-publishing/kind.yml:17
(Diff revision 1)
> +kind-dependencies:
> +   - release-uptake-monitoring
> +
> +job-defaults:
> +   description: Schedule publishing in balrog
> +   worker-type: buildbot-bridge/buildbot-bridge

O, we're not using balrogscriptworker at this stage?
Attachment #8936668 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8934790 [details]
bug 1423081 - add release partials support.

https://reviewboard.mozilla.org/r/205668/#review213288

::: taskcluster/taskgraph/transforms/partials.py:125
(Diff revisions 3 - 4)
> +            else:
> +                mar_channel_id = 'firefox-mozilla-beta'
> +        elif config.params['project'] == 'mozilla-release':
> +            mar_channel_id = 'firefox-mozilla-release'
> +        elif 'esr' in config.params['project']:
> +            mar_channel_id = 'firefox-mozilla-esr'

This feels like the type of thing that should be in the yaml, otherwise we could hit confusion.... but I think this is ok if we have a followup on file.
Comment on attachment 8934798 [details]
bug 1423081 - add desktop release beetmover support.

https://reviewboard.mozilla.org/r/205684/#review213292

::: taskcluster/taskgraph/transforms/beetmover_repackage.py:361
(Diff revisions 3 - 4)
>      for job in jobs:
>          if not is_valid_beetmover_job(job):
> -            raise NotImplementedError("Beetmover_repackage must have five dependencies.")
> +            print(job['dependencies'])
> +            raise NotImplementedError(
> +                "{}: Beetmover_repackage must have five dependencies.".format(job['label'])
> +            )

nit: debugging info in a print...
Comment on attachment 8934800 [details]
bug 1423081 - desktop release update/final verify support.

https://reviewboard.mozilla.org/r/205688/#review213294

::: commit-message-59eaf:3
(Diff revisions 3 - 4)
> -bug 1423081 - desktop release final verify support. r=callek
> +bug 1423081 - desktop release update/final verify support. r=callek
>  
> -MozReview-Commit-ID: 53TNzBXIUYh
> +MozReview-Commit-ID: AJsterLOwYg

This interdiff is harder because https://reviewboard.mozilla.org/r/205686/diff/3#index_header got folded into it....

Let me know if there are any specific changes here you want me to peek at, otherwise I'm just high-level skimming
Comment on attachment 8934806 [details]
bug 1423081 - snap support.

https://reviewboard.mozilla.org/r/205700/#review213296

::: taskcluster/docker/firefox-snap/runme.sh:82
(Diff revision 4)
>  # Upload Beta snaps to Ubuntu Snap Store (No channel)
>  # TODO: Add a release channel once ready for broader audience
>  # TODO: Don't filter out non-beta releases
>  # TODO: Parametrize channel depending on beta vs release
>  # TODO: Make this part an independent task
> -if [[ $VERSION =~ ^[0-9]+\.0b[0-9]+$ ]]; then
> +if [ "$PUSH_TO_CHANNEL" != "" ]; then

Would probably be better to detect "edge" here rather than != blank being "edge". So an invalid channel doesn't push (or attempt to push)
Comment on attachment 8934806 [details]
bug 1423081 - snap support.

https://reviewboard.mozilla.org/r/205700/#review213296

> Would probably be better to detect "edge" here rather than != blank being "edge". So an invalid channel doesn't push (or attempt to push)

As mentioned in channel,

a) this is a secret URL. If a non-edge PUSH_TO_CHANNEL is specified, the secret won't exist.
b) if someone adds the non-edge secret to the secrets service and updates the PUSH_TO_CHANNEL to that same non-edge string, likely they'll want to ship. If they don't, they've (accidentally?) taken 2 steps towards shipping but will fail on missing the secrets scope.
c) if they add the non-edge secret to the secrets service, update the PUSH_TO_CHANNEL to that same non-edge string, add the non-edge secret scope to the task definition, they've made 3 ['accidental'?] steps towards shipping, but the decision task will fail for missing the scope.
d) if they do all the above and add the scope to the decision task, I would be very hard pressed to say that person should have been protected from themselves.

Dropping this issue.
Comment on attachment 8934815 [details]
bug 1423081 - binary transparency kind.

https://reviewboard.mozilla.org/r/205718/#review213302
Attachment #8934815 - Flags: review?(bugspam.Callek) → review+
I've looked through this whole patch queue, and am happy with its outcome. feel free to land once we resolve the last few comments I've had
Comment on attachment 8934798 [details]
bug 1423081 - add desktop release beetmover support.

https://reviewboard.mozilla.org/r/205684/#review213292

> nit: debugging info in a print...

fixed on maple.
Comment on attachment 8934790 [details]
bug 1423081 - add release partials support.

https://reviewboard.mozilla.org/r/205668/#review213288

> This feels like the type of thing that should be in the yaml, otherwise we could hit confusion.... but I think this is ok if we have a followup on file.

I looked into this and I don't think it's reasonable to block on it. Note that it's two levels of by- statements that we'd need (one for project, one for devedition vs. firefox on Beta), and I couldn't find a reasonable way of doing this in partials/kind.yml. Adding them to the job-template block does nothing, because that block isn't processed until the task.py transform, which happens after partials.py.

If you have any ideas as to how to improve this, I'm happy to hear them - I'm a bit out of my depth.