Gated by go/no-go.

Comment on attachment 8923243 [details] Bug 1412690 - Fennec mark release as shipped in-tree task. https://reviewboard.mozilla.org/r/194418/#review199366

Comment on attachment 8923244 [details] Bug 1412690 - Fennec bouncer aliases in-tree task. https://reviewboard.mozilla.org/r/194420/#review199370

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

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

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.

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)

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 on attachment 8923246 [details] Bug 1412690 - Fennec uptake monitoring in-tree task. https://reviewboard.mozilla.org/r/194424/#review199402

Comment on attachment 8923247 [details] Bug 1412690 - fennec version bump in-tree task. https://reviewboard.mozilla.org/r/194426/#review199404

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 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 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 on attachment 8923250 [details] Bug 1412690 - fennec per task notifications. https://reviewboard.mozilla.org/r/194432/#review199414

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 on attachment 8923239 [details] bug 1412690 - Re-add bouncer config files. https://reviewboard.mozilla.org/r/194410/#review199552

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 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 on attachment 8923248 [details] bug 1412690 - stop building android nightlies on push. https://reviewboard.mozilla.org/r/194428/#review199574

(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 on attachment 8923240 [details] bug 1412690 - allow for relpro target_tasks_method. https://reviewboard.mozilla.org/r/194412/#review199596

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 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/

(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

(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 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 on attachment 8923245 [details] bug 1412690 - add maple fennec configs. https://reviewboard.mozilla.org/r/194422/#review199628

Comment on attachment 8923532 [details] bug 1412690 - Fix staging bouncer configs. https://reviewboard.mozilla.org/r/194656/#review199680

Comment on attachment 8923530 [details] bug 1412690 - address review comments. https://reviewboard.mozilla.org/r/194648/#review199684

Pushed by asasaki@mozilla.com: 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

Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6f8f282d662d followup, add missing file

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 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 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)

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 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 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 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 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 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 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 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.

(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.

(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 .

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.

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".

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

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

(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.

(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