Closed Bug 1332506 Opened 8 years ago Closed 8 years ago

specifcation for public/actions.json

Categories

(Firefox Build System :: Task Configuration, task)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla54

People

(Reporter: jonasfj, Assigned: jonasfj)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1332506 - Spec for in-tree treeherder actions.
59 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1332506 - Treeherder actions defined in-tree.
59 bytes, text/x-review-board-request
dustin
: review+
Details
@wlach, review/feedback/comments on if you have any: https://reviewboard.mozilla.org/r/105914/diff/1 I couldn't figure out how to added a second reviewer to mozreview after pushing :)
Flags: needinfo?(wlachance)
@wlach, Before you start implementing please have a look at: https://gist.github.com/jonasfj/c2e7cd4e0602073cb5d0e227fc4c3ec2 It implements action filtering, schema validation and template language as well as some code demonstrating how actions should be triggered. Ideally that should make it easy to implement treeherder support. - Consider it pseudo code, I doubt it'll work without some tests :)
Comment on attachment 8828588 [details] Bug 1332506 - Spec for in-tree treeherder actions. https://reviewboard.mozilla.org/r/105914/#review107124 ::: taskcluster/docs/action-spec.rst:7 (Diff revision 1) > +==================== > +This document specifies how actions exposed by the *decision task* are to be > +presented and triggered from Treeherder, or similar user interfaces. > + > +The *decision task* creates an artifact ``public/actions.json`` to be consumed > +by Treeherder. The `public/actions.json`` file specifies actions that can be *Just* treeherder? :) ::: taskcluster/docs/action-spec.rst:23 (Diff revision 1) > +This has two purposes: > + > + 1. Facilitate development of utility actions/tools in-tree, and, > + 2. Strongly decouple build/test configuration from Treeherder. > + > +For details on how define custom actions in-tree, refer to **INSERT-REF**, I assume there is something to be referred to here. :) ::: taskcluster/docs/action-spec.rst:104 (Diff revision 1) > +Action Input > +------------ > +An action can take JSON input, the input format accepted by an action is > +specified using a JSON schema. This schema is specified with by the actions > +``schema`` property. For details on how to write a JSON schema refer to the > +`specification <http://json-schema.org/>`_. I would remove the last sentence and just link directly to JSON schema in the `using a...` clause. ::: taskcluster/docs/action-spec.rst:107 (Diff revision 1) > +specified using a JSON schema. This schema is specified with by the actions > +``schema`` property. For details on how to write a JSON schema refer to the > +`specification <http://json-schema.org/>`_. > + > +User interfaces for triggering actions, like Treeherder, are expected to provide > +means to supply JSON input that satisfies this schema. These interfaces are also `are expected to provide means to supply` -> `are expected to provide` ::: taskcluster/docs/action-spec.rst:133 (Diff revision 1) > +An action with ``kind: 'task'`` is backed by an action task. That is when > +triggered the action creates a new task, and this is the result of the task. > +The task created by the action, may be useful in its own right, or it may > +simplify trigger in-tree scripts that creates new tasks. This way in-tree > +scripts can be triggered to execute complicated actions such as backfilling. > + I think it would be helpful to put in some kind of paragraph/section which went through end-to-end of how this would be expected to work: 1. Find a task that you want to perform an action on, verify that it has tags that match something in actions.json 2. Create a new action task with the taskId, taskGroupId, and other options as template parameters. 3. Submit that to the taskcluster queue One key part that was not obvious to me at first was how the taskId and taskGroupId are crucial *context* to the generation of an appropriate action tasks. It's from these elements that we would (for example) know to retrigger a mochitest-gl job and not a linux64 build. :) ::: taskcluster/docs/actions-schema.yml:18 (Diff revision 1) > +type: object > +properties: > + version: > + enum: [1] > + type: integer > + actions: Doesn't `actions` need a type? I assume it should be 'list'.
Comment on attachment 8828588 [details] Bug 1332506 - Spec for in-tree treeherder actions. https://reviewboard.mozilla.org/r/105914/#review107166 Thanks for writing this, it's a great start, some comments inline. I think it would make sense to get at least the in-tree parts of this working before committing to the specification.
(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #2) > @wlach, review/feedback/comments on if you have any: > https://reviewboard.mozilla.org/r/105914/diff/1 > > I couldn't figure out how to added a second reviewer to mozreview after > pushing :) Left some comments. Please keep me posted as you update this.
Flags: needinfo?(wlachance)
Assignee: nobody → jopsen
Comment on attachment 8828588 [details] Bug 1332506 - Spec for in-tree treeherder actions. https://reviewboard.mozilla.org/r/105914/#review107292 The content of this (as in, the structure of actions.json) looks great -- comments are all on the narrative. ::: taskcluster/docs/action-spec.rst:31 (Diff revision 2) > +Specification of Actions > +------------------------ > +The *decision task* creates an artifact ``public/actions.json`` which contains > +a list of actions to be presented in the user-interface. > + > +Action Meta-Data Metadata ::: taskcluster/docs/action-spec.rst:46 (Diff revision 2) > +The ``description`` property contains a human readable string describing the > +action, such as what it does, how it does it, what it is useful for. This string > +is to be render as **markdown**, allowing for bullet points, links and other > +simple formatting to explain what the action does. > + > +The ``kind`` property specifies what kind of action the entry defined. ..defines ::: taskcluster/docs/action-spec.rst:53 (Diff revision 2) > +See section on *Action Kind: ``task``* below for details. > + > +Action Context > +-------------- > +Few actions are relevant in all contexts, for this reason each action specifies > +a ``context`` property. This properties specifies when an action is relevant. This property ::: taskcluster/docs/action-spec.rst:58 (Diff revision 2) > +a ``context`` property. This properties specifies when an action is relevant. > +Actions *relevant* for a task should be displayed in a context menu for the > +given task. Similarly actions *not relevant* for a given task, should not be > +display in the context menu for the given task. > + > +As a special case we say that actions for which *no relevant* contexts can This ends up requiring a lot of special-case verbiage. Could we just define context: task-group to mean that it's relevant to the task-group? It's much more readable :) ::: taskcluster/docs/action-spec.rst:103 (Diff revision 2) > + > +Action Input > +------------ > +An action can take JSON input, the input format accepted by an action is > +specified using a `JSON schema <http://json-schema.org/>`_. This schema is > +specified with by the actions ``schema`` property. action's ::: taskcluster/docs/action-spec.rst:113 (Diff revision 2) > + > +It is expected that such user interfaces will attempt to auto-generate HTML > +forms from JSON schema specified. However, a user-interface implementor may also > +decide to hand write an HTML form for a particularly common or complex JSON > +schema. As long as the input generated from the form conforms to the schema > +specified for the given action. Reiterate here that the implementor should do deep comparison of the schema. I assume that deep comparison should happen after expanding any references? If not, then the references paragraph below should specify that the target of the reference should not change. ::: taskcluster/docs/action-spec.rst:120 (Diff revision 2) > +It is perfectly legal to reference external schemas using > +constructs like ``{"$ref": "https://example.com/my-schema.json"}``, in this case > +it however strongly recommended that the external resource is available over > +HTTPS and allows CORS requests from any source. > + > +In fact, user interface implementors should feel encourage to publish schemas encouraged ::: taskcluster/docs/action-spec.rst:121 (Diff revision 2) > +constructs like ``{"$ref": "https://example.com/my-schema.json"}``, in this case > +it however strongly recommended that the external resource is available over > +HTTPS and allows CORS requests from any source. > + > +In fact, user interface implementors should feel encourage to publish schemas > +for which they have hand written input forms. So that action developers can ..forms, so that.. ::: taskcluster/docs/action-spec.rst:164 (Diff revision 2) > +4. Replacing any object on the form ``{$dumps: value}`` with the > + value of ``JSON.stringify(result)`` where ``result`` is the result > + of recursive application of rules 1-4 on `value`. > + > +.. warning:: > + The template language is currently under active development additional development and additional ::: taskcluster/docs/actions-schema.yml:125 (Diff revision 2) > + When an action have been selected in the appropriate context and > + input satisfying the `schema` (if any) has been collected. The > + action is triggered by parameterizing the task template given in > + this property, and creating the resulting task. > + > + The template is parameterized with the following variables: For this and the above, I suggest keeping only the first paragraph or two in the schema document and referring to the in-tree documentation for the remainder. This gives enough context in the schema for the reader to know whta's going on, whil avoiding duplicating the details of how parameterization works or the "in-practice" notes.
Attachment #8828588 - Flags: review?(dustin) → review+
@wlach, In the process of writing the in-tree action generation, I found that it would be useful to have a 'constants' property that could defined additional variables as constants. Mainly to reduce size when reusing large JSON object just the _decision parameters_. Thoughts? Also feel free to take a look at the proposed in-tree action registry. I'll admit that I haven't tried to generate a callback task yet, so I don't know if it'll really work :) There definitely might still be bugs in the template or the callback tasks. Perhaps I need some tests, or a minimalistic on taskcluster tools for interpreting actions.json, so I can play it in reality. I suspect in practice most actions will use the callback abstraction I wrote, so once it works a test framework is probably overkill. Note: maybe we should not pretty print the JSON in-order to reduce size, I just have 1 action and it's already 4kb... By the way, there is an actions.json file here: https://public-artifacts.taskcluster.net/LqRNJvkWQyyF4FyDPyWXbg/0/public/actions.json
Flags: needinfo?(wlachance)
Comment on attachment 8830575 [details] Bug 1332506 - Treeherder actions defined in-tree. https://reviewboard.mozilla.org/r/107020/#review108576 My comments are mostly minor Python stuff, but I would like to see the action task's definition based on `.taskcluster.yml`, as I forsee issues later if that's not done. I see that being a substantial change to the review request, so I'd like to look again. ::: taskcluster/actions/hello-action.py:22 (Diff revision 2) > + 'description': """ > +A name wish to say hello to... > +This should normally be **your name**. > + > +But you can also use the default value `'World'`. > + """.strip(), This doesn't matter here, but fyi there's a neat `textwrap` module with a `dedent` function that works sort of like `|` in yaml. ::: taskcluster/actions/registry.py:75 (Diff revision 2) > + function > + To be used as decorator for the function that builds the task template. > + The decorated function will be given decision parameters and may return > + ``None`` instead of a task template, if the action is disabled. > + """ > + global actions If you never assign to `actions`, then there's no need for a global declaration. ::: taskcluster/actions/registry.py:76 (Diff revision 2) > + To be used as decorator for the function that builds the task template. > + The decorated function will be given decision parameters and may return > + ``None`` instead of a task template, if the action is disabled. > + """ > + global actions > + assert isinstance(title, str), 'title must be a string' use `basestring` instead of `string` to allow for unicode strings (same below) ::: taskcluster/actions/registry.py:80 (Diff revision 2) > + global actions > + assert isinstance(title, str), 'title must be a string' > + assert isinstance(description, str), 'description must be a string' > + assert isinstance(order, int), 'order must be an integer' > + assert is_json(schema), 'schema must be a JSON compatible object' > + mem = {"registered": False} # workaround nonlocal missing in 2.x You never actually set this to True! This is awkward in Python, it's true. A better solution might be to just look for an existing `Action` in `actions`, or to use a class. Or just not check at all -- it'd take some work to misuse the function in this fashion. In cases where I *do* need the intermediate state, I would use a class: ``` class TaskActionDecorator(object): def __init__(title, description, order, context, available, schema): self.registered = False assert ... self.action = Action(...) ... def __call__(self, task_template_builder): assert not self.registered self.registered = True action.task_template_builder = task_template_builder actions.append(self.action) ... register_task_action = TaskActionDecorator ``` ::: taskcluster/actions/registry.py:144 (Diff revision 2) > + global callbacks > + mem = {"registered": False} # workaround nonlocal missing in 2.x Same here -- no need for the global, and while you do set `mem['registered']` to True this time, I still argue it's not necessary. ::: taskcluster/actions/registry.py:154 (Diff revision 2) > + @register_task_action(title, description, order, context, schema) > + def build_callback_action_task(parameters): I like the pairing of the action task and the callback here! ::: taskcluster/actions/registry.py:182 (Diff revision 2) > + 'tc-treeherder.v2.{}.{}.{}'.format( > + parameters['project'], parameters['head_rev'], parameters['pushlog_id']), > + 'tc-treeherder-stage.v2.{}.{}.{}'.format( > + parameters['project'], parameters['head_rev'], parameters['pushlog_id']), > + ], > + 'payload': { I'd really prefer not to bury this task definition in code here, especially since it shares so much with the decision task specified in `.taskcluster.yml`. We've historically had some trouble keeping that file and `action.yml` in sync, e.g., changing docker images or adding run-task arguments. Sadly, those manifest as new bugs filed hours or days after something lands that changes `.taskcluster.yml`, so they end up frustrating users who want to perform some action and cannot until a fix lands and is merged about. Could you factor the cron-job creation code from https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/cron/decision.py#31 and share it? Then when we change the format of `.taskcluster.yml`, we only have one bit of code to change to handle the new format. ::: taskcluster/actions/registry.py:204 (Diff revision 2) > + }, > + 'features': { > + 'taskclusterProxy': True, > + 'chainOfTrust': True, > + }, > + 'image': docker_image('decision'), We don't build the decision image dynamically, so there won't be a dependent task to find here. ::: taskcluster/actions/registry.py:212 (Diff revision 2) > + '/home/worker/bin/run-task', '--vcs-checkout=/home/worker/checkouts/gecko', > + '--', 'bash', '-cx', > + """\ > +cd /home/worker/checkouts/gecko && > +ln -s /home/worker/artifacts artifacts && > +./mach --log-no-times taskgraph action-callback""" + ' '.join([ space before """ ::: taskcluster/actions/registry.py:229 (Diff revision 2) > + ]), > + ], > + }, > + 'extra': { > + 'treeherder': { > + 'symbol': 'A', This should probably be a split group(symbol), where that information is provided as one of the decorator argments -- similar to how we've provided a treeherder-symbol in `.cron.yml` (https://dxr.mozilla.org/mozilla-central/source/.cron.yml#21). Then users will see something like `A(bf bf hw)` for a push with two backfill actions and a hello-world action. ::: taskcluster/actions/registry.py:279 (Diff revision 2) > + > +def trigger_action_callback(): > + """ > + Trigger action callback using arguments from environment variables. > + """ > + global callbacks no need for global ::: taskcluster/docs/in-tree-actions.rst:29 (Diff revision 2) > + description="Simple **proof-of-concept** callback action", > + order=10000, # Order in which it should appear relative to other actions > + ) > + def hello_world_action(parameters, input, task_group_id, task_id, task): > + # parameters is an instance of taskgraph.parameters.Parameters > + # it carries decision task parameters. Will these always be identical to the parameters.yml determined in the original decision task, or are they created anew from the `./mach taskgraph action-callback` invocation? I can see merit to either one, but we should be specific. ::: taskcluster/docs/in-tree-actions.rst:96 (Diff revision 2) > +This is done by passing a JSON schema as the ``schema`` parameter. > + > +When designing a schema for the input it is important to exploit as many of the > +JSON schema validation features as reasonably possible. Furthermore, it is > +*strongly* encouraged that the ``title`` and ``description`` properties in > +JSON schemas is exploited to provide a details explanation of what the input s/is exploited// s/details/detailed/ ::: taskcluster/docs/in-tree-actions.rst:165 (Diff revision 2) > + @register_callback_action( > + title='Say Hello', > + description="Simple **proof-of-concept** callback action", > + order=2, > + # Define an action that is only included if this is a push to try > + available=lambda parameters: parameters.get('project', None) == 'try', perfect example :)
Attachment #8830575 - Flags: review?(dustin) → review-
Comment on attachment 8830575 [details] Bug 1332506 - Treeherder actions defined in-tree. https://reviewboard.mozilla.org/r/107020/#review108576 > use `basestring` instead of `string` to allow for unicode strings (same below) does this maintain python 3 compat? > You never actually set this to True! > > This is awkward in Python, it's true. A better solution might be to just look for an existing `Action` in `actions`, or to use a class. Or just not check at all -- it'd take some work to misuse the function in this fashion. > > In cases where I *do* need the intermediate state, I would use a class: > > ``` > class TaskActionDecorator(object): > def __init__(title, description, order, context, available, schema): > self.registered = False > assert ... > self.action = Action(...) > ... > > def __call__(self, task_template_builder): > assert not self.registered > self.registered = True > action.task_template_builder = task_template_builder > actions.append(self.action) > ... > > register_task_action = TaskActionDecorator > ``` I added `mem['registered'] = True`... > Same here -- no need for the global, and while you do set `mem['registered']` to True this time, I still argue it's not necessary. Okay, maybe we don't need it, but it's nice to have and doesn't use much space. > I'd really prefer not to bury this task definition in code here, especially since it shares so much with the decision task specified in `.taskcluster.yml`. We've historically had some trouble keeping that file and `action.yml` in sync, e.g., changing docker images or adding run-task arguments. Sadly, those manifest as new bugs filed hours or days after something lands that changes `.taskcluster.yml`, so they end up frustrating users who want to perform some action and cannot until a fix lands and is merged about. > > Could you factor the cron-job creation code from https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/cron/decision.py#31 and share it? Then when we change the format of `.taskcluster.yml`, we only have one bit of code to change to handle the new format. Following talk on IRC, we'll leave this as is. When we get json-e we'll rethink it. Note I already use `'image': docker_image('decision'),` so it's only incompatible changes to the decision task image that'll cause issues. > We don't build the decision image dynamically, so there won't be a dependent task to find here. This reads the in-tree `HASH` file... same as other prebuilt images.
Comment on attachment 8830575 [details] Bug 1332506 - Treeherder actions defined in-tree. https://reviewboard.mozilla.org/r/107020/#review108576 > Will these always be identical to the parameters.yml determined in the original decision task, or are they created anew from the `./mach taskgraph action-callback` invocation? I can see merit to either one, but we should be specific. it'll always be identical... I copy parameters into the `constants` section of `actions.json` and inject them as an env var.. Seems like the easist way to get them in :)
(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #12) > @wlach, > In the process of writing the in-tree action generation, I found that it > would be useful to have a > 'constants' property that could defined additional variables as constants. > Mainly to reduce size when reusing large JSON object just the _decision > parameters_. Thoughts? I think this could make sense, but YAGNI suggests maybe hold off until we see a strong need for it? It seems like this would be a straightforward extension of the existing schema/format.
Flags: needinfo?(wlachance)
Comment on attachment 8830575 [details] Bug 1332506 - Treeherder actions defined in-tree. https://reviewboard.mozilla.org/r/107020/#review108576 > does this maintain python 3 compat? No, but it's required to be correct here. In Python 2, "str" is only bytestrings, so you're prohibiting Unicode strings, and we have unicode literals turned on, so you'll probably see schema validation failures when supplying literal values as title. > This reads the in-tree `HASH` file... same as other prebuilt images. Yep, I was totally wrong about this one, sorry ;)
> I think this could make sense, but YAGNI suggests maybe hold off until we see a strong need for it? I need to for including parameters... I want to inject the following in all callback action tasks: https://gist.github.com/jonasfj/78db52ca25cdfca450a650201a9a8cac And if we have a lot of callback actions I see how it quickly becomes huge. Hence, deduplicating constants like that is very useful already.
Especially in the context of JSON Schemas, I think use of the name "constants" is confusing. After all, nothing is executing here so everything is constant. I suppose "template-vars" would be more accurate, although it's clumsy. I very much like the concept, though.
Comment on attachment 8830575 [details] Bug 1332506 - Treeherder actions defined in-tree. https://reviewboard.mozilla.org/r/107020/#review109476 If this lands with the name still spelled "constants", that'd be OK.
Attachment #8830575 - Flags: review?(dustin) → review+
Agreed with dustin to rename "constants" -> "variables"
seems there are open issues in mozreview and also need ship it review for the second patch
Flags: needinfo?(jopsen)
Keywords: checkin-needed
Should we wait to check this in until the treeherder support is ready?
Comment on attachment 8828588 [details] Bug 1332506 - Spec for in-tree treeherder actions. https://reviewboard.mozilla.org/r/105914/#review107292 > This ends up requiring a lot of special-case verbiage. Could we just define > > context: task-group > > to mean that it's relevant to the task-group? It's much more readable :) How about we say, if the `context` property is omitted? > For this and the above, I suggest keeping only the first paragraph or two in the schema document and referring to the in-tree documentation for the remainder. This gives enough context in the schema for the reader to know whta's going on, whil avoiding duplicating the details of how parameterization works or the "in-practice" notes. paraneterization will be deduplicated when json-e is introduced. I like having comments in the schema, as it ensures all properties are documented.
Comment on attachment 8828588 [details] Bug 1332506 - Spec for in-tree treeherder actions. https://reviewboard.mozilla.org/r/105914/#review107292 > How about we say, if the `context` property is omitted? For the record, it's verbiage for saying, if the context property isn't defined then it's relevant for the task-group. I prefer to avoid anyOf in schemas..
> seems there are open issues in mozreview and also need ship it review for the second patch Okay, I seem to have been confused... and failed to publish my replies.. @dustin, We can land on either side of TH support... it does no harm as is..
Flags: needinfo?(jopsen)
I tried to autoland this, but it was rejected as needing a rebase.
Friendly ping. :) Could we get this rebased and landed? I'd like to start work on bug 1322433, which depends on this.
Blocks: 1322433
I rebased can someone see if this will autoland? Thanks.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7862da54b60 Spec for in-tree treeherder actions. r=dustin https://hg.mozilla.org/integration/autoland/rev/b7ec9bebd877 Treeherder actions defined in-tree. r=dustin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7862da54b60 https://hg.mozilla.org/mozilla-central/rev/b7ec9bebd877
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: