Closed Bug 1470621 Opened 6 years ago Closed 6 years ago

Convert all existing actions to use hooks

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(firefox-esr60 fixed, firefox62 fixed, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1470621: make all actions use hooks
59 bytes, text/x-review-board-request
tomprince
: review+
Details
ci-configuration.patch
2.50 KB, patch
tomprince
: review+
Details | Diff | Splinter Review
Bug 1470621: actionPerm is the cb_name, not name;
59 bytes, text/x-review-board-request
tomprince
: review+
Details 
      No description provided.
Attachment #8987966 - Flags: review?(mozilla)
I can see two ways of cleaning this up:
 - allow `level` and `trust-domain` to be lists, and implicitly expand those using a cross-product
 - grant scopes with scope-grants.yml (bug 1465842)

Both can come later.
Attachment #8987969 - Flags: review?(mozilla)
Attachment #8987969 - Attachment is patch: true
Comment on attachment 8987969 [details] [diff] [review]
ci-configuration.patch

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

::: actions.yml
@@ +75,5 @@
> +
> +- trust_domain: gecko
> +  level: 1
> +  action_perm: cancel-all
> +  groups: [releng, vpn_sheriff]

I think we probably want to give everyone with level-1 permissions permission the ability to cancel all jobs on level-1 trees. We want people able to cancel all jobs on a try push, for example. (And possibly level-2 as well).
Attachment #8987969 - Flags: review?(mozilla) → review+
Comment on attachment 8987966 [details]
Bug 1470621: make all actions use hooks

https://reviewboard.mozilla.org/r/253236/#review260082

This looks good. I think we want to handle rel-pro in a separate bug, given that two callers haven't been updated yet.

Also, we should test all the switched actions before landing this.

::: taskcluster/taskgraph/actions/release_promotion.py:58
(Diff revision 1)
>  
>  @register_callback_action(
>      name='release-promotion',
>      title='Release Promotion',
> +    kind='hook',
> +    generic=False,

Lets split this one into a separate bug. There are at least a couple of places that need to be updated to handle hooks before this bit can land:

- https://hg.mozilla.org/build/tools/file/61c5335d64e2/lib/python/kickoff/actions.py#l37
- https://github.com/mozilla-releng/services/blob/master/src/shipit_workflow/shipit_workflow/tasks.py
Attachment #8987966 - Flags: review?(mozilla) → review+
(applied with ci-admin)
The hooks are erroring out because data.user.taskId doesn't exist..
  https://github.com/mozilla/treeherder/pull/3728
Attached file action_perm_cb_name.patch (obsolete) — 
action_perm needs to be the cb_name for the specific callback, not the name (which might correspond to a few different callbacks, e.g., retrigger works differently for mochitests and non-mochitests).
Attachment #8988544 - Flags: review?(mozilla)
Comment on attachment 8988544 [details]
action_perm_cb_name.patch

We discussed this in IRC, and are instead going to adjust the in-tree code to not expose the names of the python callbacks.
Attachment #8988544 - Flags: review?(mozilla) → review-
Testing:

[DONE] add-new-jobs
BnMdsqUySWmXFU-PWzPQew

[DONE] add-talos (context: [])
buIm9ja5RJWt_IpfKdUhOA

[DONE] cancel-all (context: []; non-generic)
LTd1HfPeTP2tB9Qm5CjKOQ

[DONE] mochitest retrigger
Ega9P6msSjGanT5mYdVz0A

[DONE] purge-caches (non-generic)
LsD_XNBNRLOaMcs6M1VZhw

[DONE] rerun
bov0-7AkRQWtBFagdogq6w

[DONE] run-missing-tests (context: [])
GGnqqUEGSyq8v2TwZ4j7Gw

The actions with context: [] need https://github.com/mozilla/treeherder/pull/3728 to land first.
Attachment #8988544 - Attachment is obsolete: true
Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

https://reviewboard.mozilla.org/r/253790/#review260524

::: taskcluster/taskgraph/actions/registry.py:144
(Diff revision 1)
>              assert 1 <= len(symbol) <= 25, 'symbol must be between 1 and 25 characters'
>          assert isinstance(symbol, basestring), 'symbol must be a string'
>  
>          assert not mem['registered'], 'register_callback_action must be used as decorator'
> -        assert cb.__name__ not in callbacks, 'callback name {} is not unique'.format(cb.__name__)
> +        if not cb_name:
> +            cb_name = cb.__name__

I tried just setting this to `name`. It looks like the mochitest retrigger is called `retrigger-mochitest-reftest-with-options` so none of them conflict.

I think we should just go with the name (but with the check that they are unique) for now. If we later want to add identically named actions with different callbacks (that presumably have disjoint sets of tasks they work on), we can add a cb_name parameter that defaults to name.
Attachment #8988547 - Flags: review?(mozilla) → review-
Names are not required to be unique -- https://docs.taskcluster.net/docs/manual/using/actions/spec#action-metadata

I'm not sure why the mochitest retrigger isn't named "retrigger".  Likely because we couldn't figure out how to make those task sets disjoint (I've been thinking of maybe adding a "precedence" to allow non-disjoint sets).

So it doesn't seem like a good idea to remove that capability now, only to re-land it later.
Well, I'd be happy with just defaulting the callback name to the name (rather than the function name). That would mean there isn't anybody explicitly setting the callback name. But that would be fine.
On reflection, I think we should make all actions generic for now (except relpromo).  It's just too much scope fiddling to try to do more than that.  So purge-cache, cancel, and cancel-all can all be generic.  We can follow up later to make those non-generic.
Comment on attachment 8988547 [details]
Bug 1470621: actionPerm is the cb_name, not name;

https://reviewboard.mozilla.org/r/253790/#review261330
Attachment #8988547 - Flags: review?(mozilla) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ceffafc6dee3
make all actions use hooks r=tomprince
https://hg.mozilla.org/integration/autoland/rev/3f4e1feb7c8e
actionPerm is the cb_name, not name; r=tomprince
https://hg.mozilla.org/mozilla-central/rev/ceffafc6dee3
https://hg.mozilla.org/mozilla-central/rev/3f4e1feb7c8e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: