1412690 - land fennec relpro patches on m-c

Status: RESOLVED FIXED

(Release Engineering :: Release Automation, enhancement)

Description

[Aki Sasaki (not active)]

Gated by go/no-go.

Comment 1

[Aki Sasaki (not active)]

Attachment: bug 1412690 - Re-add bouncer config files.

In bug 1360587, we removed bouncer.apk. We accidentally removed the
mozharness config files for bouncer submission in d72b09c9228c as well.

This patch merely adds the files back.

Review commit: https://reviewboard.mozilla.org/r/194410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194410/

Comment 2

[Aki Sasaki (not active)]

Attachment: bug 1412690 - allow for relpro target_tasks_method.

This patch creates a new `filter_beta_release_tasks` function. By
pulling this logic out of `mozilla_beta_tasks`, we can reuse it in the
release promotion `target_tasks_method`s.

Review commit: https://reviewboard.mozilla.org/r/194412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194412/

Comment 3

[Aki Sasaki (not active)]

Attachment: bug 1412690 - add {promote,publish}_{fennec,firefox} target task methods.

This hooks up the `target_tasks_method`s and the
`release_promotion_flavor`s for these four release promotion flavors.

We also add some maple support and add `version_display.txt` to the
decision task sparse checkout so we can read the version number during
decision/action task time.

Review commit: https://reviewboard.mozilla.org/r/194414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194414/

Comment 4

[Aki Sasaki (not active)]

Attachment: bug 1412690 - add fennec release bouncer sub.

This patch adds the first releasetask as a new kind. To support this, we
added a new `release-promotion` flag in the buildbot job. If this is
set, we use the new `bb_release_worker` function; otherwise we fall back
to the `bb_ci_worker` function (this is the old behavior, factored out
into a separate function).

We also added `build_number` and `release_promotion` attributes in the
task definition.  Finally, `build_number` now defaults to 1, allowing us
to create the task graph locally without forcing us to set
`BUILD_NUMBER` in the environment.

Review commit: https://reviewboard.mozilla.org/r/194416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194416/

Comment 5

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - Fennec mark release as shipped in-tree task.

This patch adds the initial `release-mark-as-shipped` kind, and adds it to
the `publish_fennec` `target_tasks_method`.

Review commit: https://reviewboard.mozilla.org/r/194418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194418/

Comment 6

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - Fennec bouncer aliases in-tree task.

This patch adds the initial `release-bouncer-aliases` kind, and adds it
to the `publish_fennec` `target_tasks_method`.

It also adds the ability to specify the `tuxedo_server_url`
`by-project`.

Review commit: https://reviewboard.mozilla.org/r/194420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194420/

Comment 7

[Aki Sasaki (not active)]

Attachment: bug 1412690 - add maple fennec configs.

This patch adds fennec staging and production relpro configs.

Review commit: https://reviewboard.mozilla.org/r/194422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194422/

Comment 8

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - Fennec uptake monitoring in-tree task.

This patch adds the initial `release-uptake-monitoring` kind, and adds
it to the `publish_fennec` `target_tasks_method`.

Review commit: https://reviewboard.mozilla.org/r/194424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194424/

Comment 9

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - fennec version bump in-tree task.

This patch adds the initial `release-version-bump` kind, and adds it to
the `publish_fennec` `target_tasks_method`.

It also adds support for `next_version`.

Review commit: https://reviewboard.mozilla.org/r/194426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194426/

Comment 10

[Aki Sasaki (not active)]

Attachment: bug 1412690 - stop building android nightlies on push.

We were building some Android nightly platforms on push, because we
didn't add them explicitly to the remove list.

I think longer term, we'll want to have `product` and
`release_promotion_action` attributes set in a standard manner, that we
can use to filter these things more intelligently.

Review commit: https://reviewboard.mozilla.org/r/194428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194428/

Comment 11

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - add release-bouncer-sub to publish_fennec's previous_graph_kinds.

We currently use `previous_graph_kinds` as a pointer to use a previously
built graph's task instead of generating a new task. We were re-running
`release-bouncer-sub` in the `publish_fennec` release promotion flavor
because we hadn't specified `release-bouncer-sub` as a
`previous_graph_kind`; this patch fixes that error.

Review commit: https://reviewboard.mozilla.org/r/194430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194430/

Comment 12

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - fennec per task notifications.

This patch adds per-task pulse notifications, as well as general support
to handle them.

Longer term we may move away from pulse-notifications, but this allows
us to proceed with pulse-notifications until that time.

Review commit: https://reviewboard.mozilla.org/r/194432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194432/

Comment 13

[Aki Sasaki (not active)]

Attachment: bug 1412690 - beetmover-cdns.

This patch adds the beetmover-cdns kind, and adds it to
`publish_fennec`.

This was the first non-buildbot-bridge, non-dummy relpro task, so this
needed a new transform.

This patch also updates the `previous_graph_kinds` and updates the
beetmover scopes in scriptworker.py.

Review commit: https://reviewboard.mozilla.org/r/194434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194434/

Comment 14

[Aki Sasaki (not active)]

Attachment: Bug 1412690 - fennec release driver emails.

This patch adds the `release-notify-promote` and `release-notify-publish` kinds. It also genericizes all the notifications, and updates the kinds that use those notifications.

Review commit: https://reviewboard.mozilla.org/r/194436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194436/

Comment 15

[Aki Sasaki (not active)]

Attachment: bug 1412690 - add task dependencies.

This patch adds the `release_deps` transform, which adds every
kind-dependency task that has the same product as a task dependency
(with some exceptions).

This patch made it clear that we need a standard way of defining
product.

Review commit: https://reviewboard.mozilla.org/r/194438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194438/

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 8923243 [details]
Bug 1412690 - Fennec mark release as shipped in-tree task.

https://reviewboard.mozilla.org/r/194418/#review199366

Comment 17

[Aki Sasaki (not active)]

Comment on attachment 8923244 [details]
Bug 1412690 - Fennec bouncer aliases in-tree task.

https://reviewboard.mozilla.org/r/194420/#review199370

Comment 18

[Aki Sasaki (not active)]

Attachment: mc-desktop-nightly-target-graph.json.diff

No diffs in the m-c on-push graph.
In the m-c nightly desktop graph, we get product: firefox diffs.

Comment 19

[Aki Sasaki (not active)]

Attachment: mc-fennec-nightly-target-graph.json.diff

In the mc nightly fennec graph, we get product: fennec diffs.

Comment 20

[Aki Sasaki (not active)]

Attachment: mb-onpush-target-graph.json.diff

In the m-b on-push graph, we stop building android nightlies. These are mainly the flavors of android that were added after the initial TC graph (aarch, old-id), so we didn't exclude them from building.

Comment 21

[Aki Sasaki (not active)]

Attachment: mb-fennec-candidates-target-graph.json.diff

In candidates_fennec, we:

- add next_version
- add a release-notify-promote-fennec task
- add a release-bouncer-sub task
- remove push-apk and push-apk-breakpoint (these are run in publish_fennec)

Comment 22

[Aki Sasaki (not active)]

Latest promote graph: https://tools.taskcluster.net/groups/OjYNEsvOSASfVR_mG8VvZg
Latest publish graph: https://tools.taskcluster.net/groups/OnBJDopuSZ-12I1sGPbg_g
 - push-apk on maple is a dummy task, so won't resolve without a human resolving it
 - staging bouncer stuff will break

Comment 23

[Aki Sasaki (not active)]

Comment on attachment 8923246 [details]
Bug 1412690 - Fennec uptake monitoring in-tree task.

https://reviewboard.mozilla.org/r/194424/#review199402

Comment 24

[Aki Sasaki (not active)]

Comment on attachment 8923247 [details]
Bug 1412690 - fennec version bump in-tree task.

https://reviewboard.mozilla.org/r/194426/#review199404

Comment 25

[Aki Sasaki (not active)]

Comment on attachment 8923249 [details]
Bug 1412690 - add release-bouncer-sub to publish_fennec's previous_graph_kinds.

https://reviewboard.mozilla.org/r/194430/#review199406

Comment 26

[Aki Sasaki (not active)]

Comment on attachment 8923250 [details]
Bug 1412690 - fennec per task notifications.

https://reviewboard.mozilla.org/r/194432/#review199408

::: taskcluster/ci/release-bouncer-aliases/kind.yml:34
(Diff revision 1)
>                      mozilla-release: https://bounceradmin.mozilla.com/api
>                      maple: https://bounceradmin.stage.allizom.org/api
>                      default: http://localhost/api
> +         notifications:
> +            completed:
> +               - releasetasks

Hm, should we remove the `completed` notifications here and now?

We could also remove them later, or in pulse-notify itself.

Comment 27

[Aki Sasaki (not active)]

Comment on attachment 8923252 [details]
Bug 1412690 - fennec release driver emails.

https://reviewboard.mozilla.org/r/194436/#review199410

::: taskcluster/ci/release-bouncer-aliases/kind.yml:34
(Diff revision 1)
>                      mozilla-release: https://bounceradmin.mozilla.com/api
>                      maple: https://bounceradmin.stage.allizom.org/api
>                      default: http://localhost/api
> -         notifications:
> +      notifications:
> -            completed:
> +         completed:
> -               - releasetasks
> +            by-project:

I suppose we'd also have to adjust this commit. Let's land these as-is, and followup if we want.

Comment 28

[Aki Sasaki (not active)]

Comment on attachment 8923250 [details]
Bug 1412690 - fennec per task notifications.

https://reviewboard.mozilla.org/r/194432/#review199414

Comment 29

[Rok Garbas [:garbas]]

Comment on attachment 8923250 [details]
Bug 1412690 - fennec per task notifications.

https://reviewboard.mozilla.org/r/194432/#review199504

::: taskcluster/ci/release-bouncer-aliases/kind.yml:34
(Diff revision 1)
>                      mozilla-release: https://bounceradmin.mozilla.com/api
>                      maple: https://bounceradmin.stage.allizom.org/api
>                      default: http://localhost/api
> +         notifications:
> +            completed:
> +               - releasetasks

I would land this first and then remove it as an after step. I added this in the cleanup/future column in trello (also reffering to Bug 1412014)

Comment 30

[Rail Aliiev [:rail]]

Comment on attachment 8923239 [details]
bug 1412690 - Re-add bouncer config files.

https://reviewboard.mozilla.org/r/194410/#review199552

Comment 31

[Rail Aliiev [:rail]]

Comment on attachment 8923242 [details]
bug 1412690 - add fennec release bouncer sub.

https://reviewboard.mozilla.org/r/194416/#review199564

::: taskcluster/taskgraph/util/scriptworker.py:417
(Diff revision 1)
>          dict: containing both `build_number` and `version`.  This can be used to
>              update `task.payload`.
>      """
>      release_config = {}
> -    if config.params['target_tasks_method'] in BEETMOVER_RELEASE_TARGET_TASKS:
> -        build_number = str(os.environ.get("BUILD_NUMBER", ""))
> +    if force or config.params['target_tasks_method'] in BEETMOVER_RELEASE_TARGET_TASKS:
> +        build_number = str(os.environ.get("BUILD_NUMBER", 1))

I wonder if we should be stricter here and require BUILD_NUMBER to be set explicitly rather than falling back to 1?

Comment 32

[Rail Aliiev [:rail]]

Comment on attachment 8923245 [details]
bug 1412690 - add maple fennec configs.

https://reviewboard.mozilla.org/r/194422/#review199568

::: testing/mozharness/configs/single_locale/maple_android-api-16.py:66
(Diff revision 1)
> +    # Balrog
> +    "build_target": "Android_arm-eabi-gcc3",
> +
> +    # Mock
> +    "mock_target": "mozilla-centos6-x86_64-android",
> +    "mock_packages": ['autoconf213', 'python', 'zip', 'mozilla-python27-mercurial', 'git', 'ccache',

Looks like we have a lot of unused stuff here, but probably we need a separate bug to clean up the configs.

Comment 33

[Rail Aliiev [:rail]]

Comment on attachment 8923248 [details]
bug 1412690 - stop building android nightlies on push.

https://reviewboard.mozilla.org/r/194428/#review199574

Comment 34

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #31)
> Comment on attachment 8923242 [details]
> bug 1412690 - add fennec release bouncer sub.
> 
> https://reviewboard.mozilla.org/r/194416/#review199564
> 
> ::: taskcluster/taskgraph/util/scriptworker.py:417
> (Diff revision 1)
> >          dict: containing both `build_number` and `version`.  This can be used to
> >              update `task.payload`.
> >      """
> >      release_config = {}
> > -    if config.params['target_tasks_method'] in BEETMOVER_RELEASE_TARGET_TASKS:
> > -        build_number = str(os.environ.get("BUILD_NUMBER", ""))
> > +    if force or config.params['target_tasks_method'] in BEETMOVER_RELEASE_TARGET_TASKS:
> > +        build_number = str(os.environ.get("BUILD_NUMBER", 1))
> 
> I wonder if we should be stricter here and require BUILD_NUMBER to be set
> explicitly rather than falling back to 1?

That means we have to set BUILD_NUMBER in the environment when we're testing via commandline locally. I'd rather the action require it.

Comment 35

[Rail Aliiev [:rail]]

Comment on attachment 8923240 [details]
bug 1412690 - allow for relpro target_tasks_method.

https://reviewboard.mozilla.org/r/194412/#review199596

Comment 36

[Rail Aliiev [:rail]]

Comment on attachment 8923241 [details]
bug 1412690 - add {promote,publish}_{fennec,firefox} target task methods.

https://reviewboard.mozilla.org/r/194414/#review199554

::: taskcluster/taskgraph/actions/release_promotion.py:40
(Diff revision 1)
> +            'build', 'build-signing', 'repackage', 'repackage-signing',
> +        ],
> +        'do_not_optimize': [],
> +    },
> +    'promote_firefox': {
> +        'target_tasks_method': '%(project)s_desktop_promotion',

Should this be %(product)s instead of %(project)s?

::: taskcluster/taskgraph/util/attributes.py
(Diff revision 1)
>  
>  TRUNK_PROJECTS = INTEGRATION_PROJECTS | {'mozilla-central', }
>  
>  RELEASE_PROJECTS = {
>      'mozilla-central',
> -    'mozilla-aurora',

yay!

Comment 37

[Rail Aliiev [:rail]]

Comment on attachment 8923251 [details]
bug 1412690 - beetmover-cdns.

https://reviewboard.mozilla.org/r/194434/#review199614

::: taskcluster/taskgraph/transforms/beetmover_cdns.py:74
(Diff revision 1)
> +
> +        bucket_scope = get_beetmover_bucket_scope(config)
> +        action_scope = get_beetmover_action_scope(config)
> +
> +        # TODO
> +        dependencies = {}

I assume this means that we rely on humans to wait until the previous action task graph finishes.

::: taskcluster/taskgraph/transforms/task.py:473
(Diff revision 1)
>              Required('locale'): basestring,
>          }],
>      }, {
> +        Required('implementation'): 'beetmover-cdns',
> +
> +        # the maximum time to spend signing, in seconds

A nit: s/signing/running the script/

Comment 38

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #36)
> Comment on attachment 8923241 [details]
> bug 1412690 - add {promote,publish}_{fennec,firefox} target task methods.
> 
> https://reviewboard.mozilla.org/r/194414/#review199554
> 
> ::: taskcluster/taskgraph/actions/release_promotion.py:40
> (Diff revision 1)
> > +            'build', 'build-signing', 'repackage', 'repackage-signing',
> > +        ],
> > +        'do_not_optimize': [],
> > +    },
> > +    'promote_firefox': {
> > +        'target_tasks_method': '%(project)s_desktop_promotion',
> 
> Should this be %(product)s instead of %(project)s?

Currently, it's project.
maple_desktop_promotion. That probably means we need to fix this line to be mozilla-beta_desktop_promotion though =\
https://hg.mozilla.org/projects/maple/file/tip/taskcluster/taskgraph/target_tasks.py#l305

Comment 39

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #37)
> Comment on attachment 8923251 [details]
> bug 1412690 - beetmover-cdns.
> 
> https://reviewboard.mozilla.org/r/194434/#review199614
> 
> ::: taskcluster/taskgraph/transforms/beetmover_cdns.py:74
> (Diff revision 1)
> > +
> > +        bucket_scope = get_beetmover_bucket_scope(config)
> > +        action_scope = get_beetmover_action_scope(config)
> > +
> > +        # TODO
> > +        dependencies = {}
> 
> I assume this means that we rely on humans to wait until the previous action
> task graph finishes.

Dependencies are added in a later patch.

> ::: taskcluster/taskgraph/transforms/task.py:473
> (Diff revision 1)
> >              Required('locale'): basestring,
> >          }],
> >      }, {
> > +        Required('implementation'): 'beetmover-cdns',
> > +
> > +        # the maximum time to spend signing, in seconds
> 
> A nit: s/signing/running the script/

Sure. I think we can fix this, mozilla-beta_desktop_promotion, and maybe the maple tuxedo url that mtabara landed.

Comment 40

[Rail Aliiev [:rail]]

Comment on attachment 8923253 [details]
bug 1412690 - add task dependencies.

https://reviewboard.mozilla.org/r/194438/#review199620

::: taskcluster/ci/beetmover-cdns/kind.yml:13
(Diff revision 1)
> +   - taskgraph.transforms.release_deps:transforms
>     - taskgraph.transforms.beetmover_cdns:transforms
>     - taskgraph.transforms.task:transforms
>  
> +kind-dependencies:
> +   - beetmover-checksums

Yay for dependencies!

Comment 41

[Rail Aliiev [:rail]]

Comment on attachment 8923245 [details]
bug 1412690 - add maple fennec configs.

https://reviewboard.mozilla.org/r/194422/#review199628

Comment 42

[Aki Sasaki (not active)]

Attachment: bug 1412690 - address review comments.

Review commit: https://reviewboard.mozilla.org/r/194648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194648/

Comment 43

[Aki Sasaki (not active)]

Attachment: bug 1412690 - Fix staging bouncer configs.

Review commit: https://reviewboard.mozilla.org/r/194650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194650/

Comment 44

[Aki Sasaki (not active)]

Attachment: bug 1412690 - Fix staging bouncer configs.

Review commit: https://reviewboard.mozilla.org/r/194656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194656/

Comment 45

[Aki Sasaki (not active)]

Comment on attachment 8923530 [details]
bug 1412690 - address review comments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194648/diff/1-2/

Comment 46

[Aki Sasaki (not active)]

Comment on attachment 8923532 [details]
bug 1412690 - Fix staging bouncer configs.

https://reviewboard.mozilla.org/r/194656/#review199680

Comment 47

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8923530 [details]
bug 1412690 - address review comments.

https://reviewboard.mozilla.org/r/194648/#review199684

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4eb3c44d1f38
Re-add bouncer config files. r=rail
https://hg.mozilla.org/integration/autoland/rev/4adbc92314b5
allow for relpro target_tasks_method. r=rail
https://hg.mozilla.org/integration/autoland/rev/6c75d2180525
add {promote,publish}_{fennec,firefox} target task methods. r=rail
https://hg.mozilla.org/integration/autoland/rev/a7f43d6357e6
add fennec release bouncer sub. r=rail
https://hg.mozilla.org/integration/autoland/rev/664aea838665
Fennec mark release as shipped in-tree task. r=aki
https://hg.mozilla.org/integration/autoland/rev/e5bc7b578258
Fennec bouncer aliases in-tree task. r=aki
https://hg.mozilla.org/integration/autoland/rev/158ce0d62fea
add maple fennec configs. r=rail
https://hg.mozilla.org/integration/autoland/rev/b1ef93141638
Fennec uptake monitoring in-tree task. r=aki
https://hg.mozilla.org/integration/autoland/rev/89cefc910447
fennec version bump in-tree task. r=aki
https://hg.mozilla.org/integration/autoland/rev/16df2b69cb50
stop building android nightlies on push. r=rail
https://hg.mozilla.org/integration/autoland/rev/e183065a1aab
add release-bouncer-sub to publish_fennec's previous_graph_kinds. r=aki
https://hg.mozilla.org/integration/autoland/rev/ab1b3bdb14f2
fennec per task notifications. r=garbas
https://hg.mozilla.org/integration/autoland/rev/3f6b1a62127a
beetmover-cdns. r=rail
https://hg.mozilla.org/integration/autoland/rev/7f4e848f8449
fennec release driver emails. r=aki
https://hg.mozilla.org/integration/autoland/rev/ee3047d12729
add task dependencies. r=rail
https://hg.mozilla.org/integration/autoland/rev/3b9a71050e4d
address review comments. r=mtabara
https://hg.mozilla.org/integration/autoland/rev/238d6bc5b058
Fix staging bouncer configs.r=aki

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f8f282d662d
followup, add missing file

Comment 50

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4eb3c44d1f38
https://hg.mozilla.org/mozilla-central/rev/4adbc92314b5
https://hg.mozilla.org/mozilla-central/rev/6c75d2180525
https://hg.mozilla.org/mozilla-central/rev/a7f43d6357e6
https://hg.mozilla.org/mozilla-central/rev/664aea838665
https://hg.mozilla.org/mozilla-central/rev/e5bc7b578258
https://hg.mozilla.org/mozilla-central/rev/158ce0d62fea
https://hg.mozilla.org/mozilla-central/rev/b1ef93141638
https://hg.mozilla.org/mozilla-central/rev/89cefc910447
https://hg.mozilla.org/mozilla-central/rev/16df2b69cb50
https://hg.mozilla.org/mozilla-central/rev/e183065a1aab
https://hg.mozilla.org/mozilla-central/rev/ab1b3bdb14f2
https://hg.mozilla.org/mozilla-central/rev/3f6b1a62127a
https://hg.mozilla.org/mozilla-central/rev/7f4e848f8449
https://hg.mozilla.org/mozilla-central/rev/ee3047d12729
https://hg.mozilla.org/mozilla-central/rev/3b9a71050e4d
https://hg.mozilla.org/mozilla-central/rev/238d6bc5b058
https://hg.mozilla.org/mozilla-central/rev/6f8f282d662d

Comment 51

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923242 [details]
bug 1412690 - add fennec release bouncer sub.

https://reviewboard.mozilla.org/r/194416/#review200056

::: taskcluster/taskgraph/transforms/task.py:383
(Diff revision 1)
> +            Optional('build_number'): int,
> +            Optional('release_promotion'): bool,
>              Extra: taskref_or_string,  # additional properties are allowed
>          },
> +        Optional('scopes'): [basestring],
> +        Optional('routes'): [basestring],

Why do these need to be included in the worker?  Just de-indenting them in the YAML file should have the same effect..

Comment 52

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923244 [details]
Bug 1412690 - Fennec bouncer aliases in-tree task.

https://reviewboard.mozilla.org/r/194420/#review200070

::: taskcluster/taskgraph/transforms/job/buildbot.py:85
(Diff revision 1)
> +    fields = [
> +        "run.properties.tuxedo_server_url",
> +    ]
> +    job = copy.deepcopy(job)
> +    for field in fields:
> +        resolve_keyed_by(job, field, field, **config.params)

The third argument should be the name of the thing on which the resolution takes place (job['name'] here, I think)

Comment 53

[Dustin J. Mitchell [:dustin] (he/him)]

I'm about to make a bunch more comments on this patchset.  These can be addressed in a follow-up bug, but I'll re-open this just so it's on the radar.

My understanding is that these patches were by garbas with aki/rail's help and reviewed by aki, so I'm not sure who will end up fixing, but at least the comments are out there..

Comment 54

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923244 [details]
Bug 1412690 - Fennec bouncer aliases in-tree task.

https://reviewboard.mozilla.org/r/194420/#review200078

::: taskcluster/taskgraph/transforms/job/buildbot.py:33
(Diff revision 1)
>      # the product to use
>      Required('product'): Any('firefox', 'mobile', 'fennec', 'devedition', 'thunderbird'),
>  
>      Optional('release-promotion'): bool,
>      Optional('routes'): [basestring],
> +    Optional('properties'): {basestring: optionally_keyed_by('project', basestring)},

Like `routes`, this can be added directly to the worker in the YAML:

```yaml
jobs:
    fennec:
        run:
            using: ...
        worker:
            properties:
                ...
```

You would then add the `resolve_keyed_by` call in the buildbot payload-generation function.

::: taskcluster/taskgraph/transforms/job/buildbot.py:103
(Diff revision 1)
>              'repository': config.params['head_repository'],
>              'revision': revision,
>          },
>          'properties': {
>              'product': product,
>          },

..doing so would require a little more care here so as not to overwrite `worker['properties']`

Comment 55

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923247 [details]
Bug 1412690 - fennec version bump in-tree task.

https://reviewboard.mozilla.org/r/194426/#review200082

::: taskcluster/taskgraph/util/scriptworker.py:422
(Diff revision 1)
> +        next_version = str(os.environ.get("NEXT_VERSION", ""))
> +        if next_version != "":
> +            release_config['next_version'] = next_version
>          build_number = str(os.environ.get("BUILD_NUMBER", 1))
>          if not build_number.isdigit():
>              raise ValueError("Release graphs must specify `BUILD_NUMBER` in the environment!")

It would be more clear that these are inputs to the taskgraph process if they were defined as Parameters.  It should be a simple change to the Parameters support to also accept values directly from env vars instead of via --options (and probably beneficial in terms of simplicity anyway).

Comment 56

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923250 [details]
Bug 1412690 - fennec per task notifications.

https://reviewboard.mozilla.org/r/194432/#review200084

::: taskcluster/taskgraph/transforms/job/buildbot.py:40
(Diff revision 1)
>      Optional('properties'): {basestring: optionally_keyed_by('project', basestring)},
> +
> +    Optional('notifications'): {
> +        Optional('completed'): Any(notification_schema, [basestring]),
> +        Optional('failed'): Any(notification_schema, [basestring]),
> +        Optional('artifact'): Any(notification_schema, [basestring]),

This case isn't handled by the code below

::: taskcluster/taskgraph/transforms/job/buildbot.py:41
(Diff revision 1)
> +
> +    Optional('notifications'): {
> +        Optional('completed'): Any(notification_schema, [basestring]),
> +        Optional('failed'): Any(notification_schema, [basestring]),
> +        Optional('artifact'): Any(notification_schema, [basestring]),
> +        Optional('exception'): Any(notification_schema, [basestring]),

Some comments would be good here.. it looks like you can either write a `notification_schema` object or a list of ids?

Comment 57

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923251 [details]
bug 1412690 - beetmover-cdns.

https://reviewboard.mozilla.org/r/194434/#review200120

::: taskcluster/taskgraph/transforms/beetmover_cdns.py:40
(Diff revision 1)
> +    Optional('job-from'): task_description_schema['job-from'],
> +    Optional('run'): {basestring: object},
> +    Optional('run-on-projects'): task_description_schema['run-on-projects'],
> +    Required('worker-type'): Any(
> +        job_description_schema['worker-type'],
> +        {'by-project': {basestring: job_description_schema['worker-type']}},

task_description_schema['worker-type'] is the same as job_description_schema['worker-type'].  It's weird to see jobs referenced here, since this kind generates tasks directly without any intervening job description.

::: taskcluster/taskgraph/transforms/beetmover_cdns.py:41
(Diff revision 1)
> +    Optional('run'): {basestring: object},
> +    Optional('run-on-projects'): task_description_schema['run-on-projects'],
> +    Required('worker-type'): Any(
> +        job_description_schema['worker-type'],
> +        {'by-project': {basestring: job_description_schema['worker-type']}},
> +    ),

optionally_keyed_by

Also, it looks like this is never actually handled?

Comment 58

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923252 [details]
Bug 1412690 - fennec release driver emails.

https://reviewboard.mozilla.org/r/194436/#review200124

I think this notification-handling is overly general in a few ways:

1. It applies to all tasks, even though it only works on BBB.  I suspect this is looking forward to a transition to using TC-notify?
2. It allows either ID's or full notification descriptions.  You can probably get by with just one of those, or by applying defaults
3. The keyed-by stuff is rather intricate

If my suspicion in #1 is correct then that can stay (although with assertions as described below).  Then you can handle #2 and #3 as a separate transform that applies resolve_keyed_by and defaults for subject/message.

You might consider applying the keyed-by at the `notifications` level, and considering `subject` and `message` to have default values:

```yaml
    notifications:
        by-project:
            maple:
                completed:
                    ids: [release-drivers-staging]
                    subject: "Fire up the griddle, we're having pancakes!"
                failed:
                    ids: [release-drivers-staging]
                    subject: "That's not a sugar maple, friend."
                exception:
                    ids: [release-drivers-staging]
                    subject: "You have drowned in a flood of warm, delicious maple syrup.  Insert a coin to try again."
            default:
                completed:
                    ids: [release-drivers]
                failed:
                    ids: [release-drivers]
                exception:
                    ids: [release-drivers]
```

It's a little more compact, and certainly simpler to implement.

It's worth noting that as currently implemented, tc-notify doesn't let you specify a message or subject.  That's open for patches of course :)

::: taskcluster/ci/release-bouncer-aliases/kind.yml:38
(Diff revision 1)
> -            completed:
> +         completed:
> -               - releasetasks
> +            by-project:
> +               maple:
> +                  - "release-drivers-staging"
> +               try:
> +                  #- "{task[tags][createdForUser]}"

I think this was what broke try?  In YAML, a `:` with nothing following it has value `null`.

::: taskcluster/taskgraph/transforms/task.py:50
(Diff revision 1)
>  taskref_or_string = Any(
>      basestring,
> -    {Required('task-reference'): basestring})
> +    {Required('task-reference'): basestring},
> +)
>  
> +notification_ids = optionally_keyed_by('project', Any(None, [basestring]))

Is None really acceptable here?

[edit] oh, right, no, it's not :)

::: taskcluster/taskgraph/transforms/task.py:54
(Diff revision 1)
>  
> +notification_ids = optionally_keyed_by('project', Any(None, [basestring]))
>  notification_schema = Schema({
>      Required("subject"): basestring,
>      Required("message"): basestring,
> -    Required("ids"): [basestring],
> +    Required("ids"): notification_ids,

Since this is general to all tasks, rather than something that will be deleted with BBB, it needs some comments -- what are id's?  What sort of parameter substitution occurs in subject and message?

::: taskcluster/taskgraph/transforms/task.py:225
(Diff revision 1)
>      # Whether the job should use sccache compiler caching.
>      Required('needs-sccache', default=False): bool,
>  
> +    # notifications
> +    Optional('notifications'): {
> +        Optional('completed'): Any(notification_schema, notification_ids),

A little commentary would be good here -- is this only sent on successful completion?

::: taskcluster/taskgraph/transforms/task.py:228
(Diff revision 1)
> +    # notifications
> +    Optional('notifications'): {
> +        Optional('completed'): Any(notification_schema, notification_ids),
> +        Optional('failed'): Any(notification_schema, notification_ids),
> +        Optional('exception'): Any(notification_schema, notification_ids),
> +    },

I'm not sure why this moves here, at the task-description level, rather than to the BBB worker schema.  Does any other worker implementation support this?  Generally when the schema allows you to express something, but that something isn't supported, you should get an error -- so if this is at the task-description level, then all payload builders except `buildbot-bridge` should be asserting that it is not present.

::: taskcluster/taskgraph/transforms/task.py:1071
(Diff revision 1)
> +            if 'name' not in task:
> +                raise Exception("task has neither a name nor a label")
> +            task['label'] = '{}-{}'.format(config.kind, task['name'])
> +        if task.get('name'):
> +            del task['name']
> +        yield task

The task schema says that a task description doesn't have a `name` attribute.  So anything providing an object with a `name` is wrong, yet with this transform such task descriptions are allowed.

It looks like it's the two new kinds which produce task descriptions directly, and which specify a `name` and not a `label`.  I think changing `name` to `label` in those `kind.yml` files is the better fix.

::: taskcluster/taskgraph/transforms/task.py:1333
(Diff revision 1)
>  
> +        notifications = task.get('notifications')
> +        if notifications:
> +            task_def['extra'].setdefault('notifications', {})
> +            for k, v in notifications.items():
> +                if isinstance(v, dict) and len(v) == 1 and v.keys()[0].startswith('by-'):

`resolve_keyed_by` already has this logic -- why is it repeated here?

Also, the schema doesn't have `optionally_keyed_by` here.

::: taskcluster/taskgraph/transforms/task.py:1336
(Diff revision 1)
> +            task_def['extra'].setdefault('notifications', {})
> +            for k, v in notifications.items():
> +                if isinstance(v, dict) and len(v) == 1 and v.keys()[0].startswith('by-'):
> +                    v = {'tmp': v}
> +                    resolve_keyed_by(v, 'tmp', 'notifications', **config.params)
> +                    v = v['tmp']

`resolve_keyed_by(task, 'notifications.{}'.format(k), task['label'], **config.params)`

::: taskcluster/taskgraph/transforms/task.py:1339
(Diff revision 1)
> +                    v = {'tmp': v}
> +                    resolve_keyed_by(v, 'tmp', 'notifications', **config.params)
> +                    v = v['tmp']
> +                if isinstance(v, list):
> +                    v = {'ids': v}
> +                    if 'completed' == k:

This conditional ladder would probably be better implemented as a constant dictionary.  That would treat as an error the case where `k` is not any of these options.  At one point in this patchset, `"artifact"` is allowed, and as written this code would silently ignore it.

::: taskcluster/taskgraph/transforms/task.py:1345
(Diff revision 1)
> +                        v.update({
> +                            "subject": "Completed: {}".format(FULL_TASK_NAME),
> +                            "message": "{} has completed successfully! Yay!".format(
> +                                FULL_TASK_NAME),
> +                        })
> +                    elif k == 'failed':

nit: inconsistent constant == variable and variable == constant

::: taskcluster/taskgraph/transforms/task.py:1357
(Diff revision 1)
> +                            "subject": "Exception: {}".format(FULL_TASK_NAME),
> +                            "message": "Uh-oh! {} resulted in an exception.".format(
> +                                FULL_TASK_NAME),
> +                        })
> +                else:
> +                    resolve_keyed_by(v, 'ids', 'notifications', **config.params)

`resolve_keyed_by(task, 'notifications.{}'.format(k), task['label'], **config.params)`

hm, that looks suspiciously like my comment above.  I think this gets resolved twice?

Comment 59

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923253 [details]
bug 1412690 - add task dependencies.

https://reviewboard.mozilla.org/r/194438/#review200152

::: taskcluster/taskgraph/transforms/beetmover_checksums.py:79
(Diff revision 1)
> +        if build_platform.startswith("android"):
> +            extra['product'] = 'fennec'
> +        elif 'devedition' in build_platform:
> +            extra['product'] = 'devedition'
> +        else:
> +            extra['product'] = 'firefox'

Why is this in extra, rather than an attribute?  We already include `product` in the `index` key (for sending to treeherder).  If this matches (I can dream, right??) then the index could potentially fetch it from attributes too.

`extra` gets put in the final task definition sent to `queue.createTask`, so it's only useful for data that is interpreted later, after the task has executed (e.g., for treeherder).

Comment 60

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8923530 [details]
bug 1412690 - address review comments.

https://reviewboard.mozilla.org/r/194648/#review200158

::: commit-message-83923:1
(Diff revision 2)
> +bug 1412690 - address review comments. r=mtabara

Ideally things like this are folded back into the affected commits.  These are relatively minor changes, but if they had been more substantial then someone (like me, just now) looking back at the revision history would see broken stuff land and not necessarily see the later fixes.

Comment 61

[Aki Sasaki (not active)]

(In reply to Dustin J. Mitchell [:dustin] from comment #60)
> Comment on attachment 8923530 [details]
> bug 1412690 - address review comments.
<snip>
> Ideally things like this are folded back into the affected commits.  These
> are relatively minor changes, but if they had been more substantial then
> someone (like me, just now) looking back at the revision history would see
> broken stuff land and not necessarily see the later fixes.

Agreed. This was my largest patch submission via mozreview and I was a bit wary of possibly breaking things.

Comment 62

[Aki Sasaki (not active)]

(In reply to Dustin J. Mitchell [:dustin] from comment #59)
> Comment on attachment 8923253 [details]
> bug 1412690 - add task dependencies.
> 
> https://reviewboard.mozilla.org/r/194438/#review200152
> 
> ::: taskcluster/taskgraph/transforms/beetmover_checksums.py:79
> (Diff revision 1)
> > +        if build_platform.startswith("android"):
> > +            extra['product'] = 'fennec'
> > +        elif 'devedition' in build_platform:
> > +            extra['product'] = 'devedition'
> > +        else:
> > +            extra['product'] = 'firefox'
> 
> Why is this in extra, rather than an attribute?  We already include
> `product` in the `index` key (for sending to treeherder).  If this matches
> (I can dream, right??) then the index could potentially fetch it from
> attributes too.
> 
> `extra` gets put in the final task definition sent to `queue.createTask`, so
> it's only useful for data that is interpreted later, after the task has
> executed (e.g., for treeherder).

Agreed!
This is in extra because the scriptworkers all have payload jsonschemas, but extra sneaks in under the radar. I think we should have product and release_promotion_flavors (or something) specified as top level attributes so we can filter in a standard fashion.
We have a trello card to do this in https://trello.com/b/EGWsGSXT/tc-migration-release-h2-2017 .

Comment 63

[Aki Sasaki (not active)]

Responding here to keep the conversations in one place:

(In reply to Dustin J. Mitchell [:dustin] from comment #51)
> Comment on attachment 8923242 [details]
> bug 1412690 - add fennec release bouncer sub.
> 
> https://reviewboard.mozilla.org/r/194416/#review200056
> 
> ::: taskcluster/taskgraph/transforms/task.py:383
> (Diff revision 1)
> > +            Optional('build_number'): int,
> > +            Optional('release_promotion'): bool,
> >              Extra: taskref_or_string,  # additional properties are allowed
> >          },
> > +        Optional('scopes'): [basestring],
> > +        Optional('routes'): [basestring],
> 
> Why do these need to be included in the worker?  Just de-indenting them in
> the YAML file should have the same effect..

Makes sense. If we do that, we'll have to add info like underscore_version and build_number to the generic route logic, but that might be preferable to reinventing logic.

Comment 64

[Dustin J. Mitchell [:dustin] (he/him)]

I was referring specifically to scopes and routes.  build_number and release_promotion make sense as part of the worker (since only this worker has "properties".

Comment 65

[Dustin J. Mitchell [:dustin] (he/him)]

Sorry, I just saw the r? on bug 1415391.

Comment 66

[Aki Sasaki (not active)]

Bug 1415391 addressed most of these comments; I opened bug 1416317 for notifications.
Marking this bug as resolved.

Comment 67

[Rok Garbas [:garbas]]

(In reply to Dustin J. Mitchell [:dustin] from comment #56)
> Comment on attachment 8923250 [details]
> Bug 1412690 - fennec per task notifications.
> 
> https://reviewboard.mozilla.org/r/194432/#review200084
> 
> ::: taskcluster/taskgraph/transforms/job/buildbot.py:40
> (Diff revision 1)
> >      Optional('properties'): {basestring: optionally_keyed_by('project', basestring)},
> > +
> > +    Optional('notifications'): {
> > +        Optional('completed'): Any(notification_schema, [basestring]),
> > +        Optional('failed'): Any(notification_schema, [basestring]),
> > +        Optional('artifact'): Any(notification_schema, [basestring]),
> 
> This case isn't handled by the code below
> 
> ::: taskcluster/taskgraph/transforms/job/buildbot.py:41
> (Diff revision 1)
> > +
> > +    Optional('notifications'): {
> > +        Optional('completed'): Any(notification_schema, [basestring]),
> > +        Optional('failed'): Any(notification_schema, [basestring]),
> > +        Optional('artifact'): Any(notification_schema, [basestring]),
> > +        Optional('exception'): Any(notification_schema, [basestring]),
> 
> Some comments would be good here.. it looks like you can either write a
> `notification_schema` object or a list of ids?

In later commits the code was moved to taskcluster/taskgraph/transforms/task.py with artifact removed. I will add a comment how to use this notification option.

Comment 68

[Rok Garbas [:garbas]]

(In reply to Dustin J. Mitchell [:dustin] from comment #58)
> Comment on attachment 8923252 [details]
> Bug 1412690 - fennec release driver emails.
> 
> https://reviewboard.mozilla.org/r/194436/#review200124
> 
> I think this notification-handling is overly general in a few ways:
> 
> 1. It applies to all tasks, even though it only works on BBB.  I suspect
> this is looking forward to a transition to using TC-notify?

notifications are only used in release promotion but nothing implementation
wise make them bound to BBB.

and you assumed correctly, I moved the implementation in task transform to
replace it with tc-notify once we reach parity with our current notification
service.

> 2. It allows either ID's or full notification descriptions.  You can
> probably get by with just one of those, or by applying defaults

ok


> 3. The keyed-by stuff is rather intricate
> 
> If my suspicion in #1 is correct then that can stay (although with
> assertions as described below).  Then you can handle #2 and #3 as a separate
> transform that applies resolve_keyed_by and defaults for subject/message.
> 
> You might consider applying the keyed-by at the `notifications` level, and
> considering `subject` and `message` to have default values:
> 
> ```yaml
>     notifications:
>         by-project:
>             maple:
>                 completed:
>                     ids: [release-drivers-staging]
>                     subject: "Fire up the griddle, we're having pancakes!"
>                 failed:
>                     ids: [release-drivers-staging]
>                     subject: "That's not a sugar maple, friend."
>                 exception:
>                     ids: [release-drivers-staging]
>                     subject: "You have drowned in a flood of warm, delicious
> maple syrup.  Insert a coin to try again."
>             default:
>                 completed:
>                     ids: [release-drivers]
>                 failed:
>                     ids: [release-drivers]
>                 exception:
>                     ids: [release-drivers]
> ```
> 
> It's a little more compact, and certainly simpler to implement.
> 

ok


> It's worth noting that as currently implemented, tc-notify doesn't let you
> specify a message or subject.  That's open for patches of course :)
> 

I just hope tc-notify is not written in Go like relengapi-proxy.


> ::: taskcluster/ci/release-bouncer-aliases/kind.yml:38
> (Diff revision 1)
> > -            completed:
> > +         completed:
> > -               - releasetasks
> > +            by-project:
> > +               maple:
> > +                  - "release-drivers-staging"
> > +               try:
> > +                  #- "{task[tags][createdForUser]}"
> 
> I think this was what broke try?  In YAML, a `:` with nothing following it
> has value `null`.
> 

yes.

> ::: taskcluster/taskgraph/transforms/task.py:50
> (Diff revision 1)
> >  taskref_or_string = Any(
> >      basestring,
> > -    {Required('task-reference'): basestring})
> > +    {Required('task-reference'): basestring},
> > +)
> >  
> > +notification_ids = optionally_keyed_by('project', Any(None, [basestring]))
> 
> Is None really acceptable here?
> 
> [edit] oh, right, no, it's not :)
> 

Ah yeah, i could use []. i see.


> ::: taskcluster/taskgraph/transforms/task.py:54
> (Diff revision 1)
> >  
> > +notification_ids = optionally_keyed_by('project', Any(None, [basestring]))
> >  notification_schema = Schema({
> >      Required("subject"): basestring,
> >      Required("message"): basestring,
> > -    Required("ids"): [basestring],
> > +    Required("ids"): notification_ids,
> 
> Since this is general to all tasks, rather than something that will be
> deleted with BBB, it needs some comments -- what are id's?  What sort of
> parameter substitution occurs in subject and message?
> 

ok


> ::: taskcluster/taskgraph/transforms/task.py:225
> (Diff revision 1)
> >      # Whether the job should use sccache compiler caching.
> >      Required('needs-sccache', default=False): bool,
> >  
> > +    # notifications
> > +    Optional('notifications'): {
> > +        Optional('completed'): Any(notification_schema, notification_ids),
> 
> A little commentary would be good here -- is this only sent on successful
> completion?
> 

ok

> ::: taskcluster/taskgraph/transforms/task.py:228
> (Diff revision 1)
> > +    # notifications
> > +    Optional('notifications'): {
> > +        Optional('completed'): Any(notification_schema, notification_ids),
> > +        Optional('failed'): Any(notification_schema, notification_ids),
> > +        Optional('exception'): Any(notification_schema, notification_ids),
> > +    },
> 
> I'm not sure why this moves here, at the task-description level, rather than
> to the BBB worker schema.  Does any other worker implementation support
> this?  Generally when the schema allows you to express something, but that
> something isn't supported, you should get an error -- so if this is at the
> task-description level, then all payload builders except `buildbot-bridge`
> should be asserting that it is not present.
> 

Notifications are task specific. Our current implementation uses old release-notify
service but once we switch to tc-notify it look the right place to put it. But
nothing so far was easy with taskgraph so I might be as well wrong.


> ::: taskcluster/taskgraph/transforms/task.py:1071
> (Diff revision 1)
> > +            if 'name' not in task:
> > +                raise Exception("task has neither a name nor a label")
> > +            task['label'] = '{}-{}'.format(config.kind, task['name'])
> > +        if task.get('name'):
> > +            del task['name']
> > +        yield task
> 
> The task schema says that a task description doesn't have a `name`
> attribute.  So anything providing an object with a `name` is wrong, yet with
> this transform such task descriptions are allowed.
> 
> It looks like it's the two new kinds which produce task descriptions
> directly, and which specify a `name` and not a `label`.  I think changing
> `name` to `label` in those `kind.yml` files is the better fix.
> 

I think I tried that but wasn't working. I'll give it another shot.


> ::: taskcluster/taskgraph/transforms/task.py:1333
> (Diff revision 1)
> >  
> > +        notifications = task.get('notifications')
> > +        if notifications:
> > +            task_def['extra'].setdefault('notifications', {})
> > +            for k, v in notifications.items():
> > +                if isinstance(v, dict) and len(v) == 1 and v.keys()[0].startswith('by-'):
> 
> `resolve_keyed_by` already has this logic -- why is it repeated here?
> 
> Also, the schema doesn't have `optionally_keyed_by` here.
> 

No idea how to use `optionally_keyed_by` or that it even existed. I'll look how to use it.


> ::: taskcluster/taskgraph/transforms/task.py:1336
> (Diff revision 1)
> > +            task_def['extra'].setdefault('notifications', {})
> > +            for k, v in notifications.items():
> > +                if isinstance(v, dict) and len(v) == 1 and v.keys()[0].startswith('by-'):
> > +                    v = {'tmp': v}
> > +                    resolve_keyed_by(v, 'tmp', 'notifications', **config.params)
> > +                    v = v['tmp']
> 
> `resolve_keyed_by(task, 'notifications.{}'.format(k), task['label'],
> **config.params)`
>

ok
 
> ::: taskcluster/taskgraph/transforms/task.py:1339
> (Diff revision 1)
> > +                    v = {'tmp': v}
> > +                    resolve_keyed_by(v, 'tmp', 'notifications', **config.params)
> > +                    v = v['tmp']
> > +                if isinstance(v, list):
> > +                    v = {'ids': v}
> > +                    if 'completed' == k:
> 
> This conditional ladder would probably be better implemented as a constant
> dictionary.  That would treat as an error the case where `k` is not any of
> these options.  At one point in this patchset, `"artifact"` is allowed, and
> as written this code would silently ignore it.
> 

ok

> ::: taskcluster/taskgraph/transforms/task.py:1345
> (Diff revision 1)
> > +                        v.update({
> > +                            "subject": "Completed: {}".format(FULL_TASK_NAME),
> > +                            "message": "{} has completed successfully! Yay!".format(
> > +                                FULL_TASK_NAME),
> > +                        })
> > +                    elif k == 'failed':
> 
> nit: inconsistent constant == variable and variable == constant
> 

ok

> ::: taskcluster/taskgraph/transforms/task.py:1357
> (Diff revision 1)
> > +                            "subject": "Exception: {}".format(FULL_TASK_NAME),
> > +                            "message": "Uh-oh! {} resulted in an exception.".format(
> > +                                FULL_TASK_NAME),
> > +                        })
> > +                else:
> > +                    resolve_keyed_by(v, 'ids', 'notifications', **config.params)
> 
> `resolve_keyed_by(task, 'notifications.{}'.format(k), task['label'],
> **config.params)`
> 
> hm, that looks suspiciously like my comment above.  I think this gets
> resolved twice?

ok