Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,4 @@ See [mechanics](mechanics.md) for more detail.
| RFC#189 | [Batch APIs for task definition, status and index path](rfcs/0189-batch-task-apis.md) |
| RFC#190 | [Queue change task / task group priority](rfcs/0190-queue-change-task-run-priority.md) |
| RFC#191 | [Worker Manager launch configurations](rfcs/0191-worker-manager-launch-configs.md) |
| RFC#196 | [Trigger Hooks from Github Service](rfcs/0196-trigger-hooks-from-github-service.md) |
113 changes: 113 additions & 0 deletions rfcs/0196-trigger-hooks-from-github-service.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# RFC 0196 - Trigger Hooks from Github Service
* Comments: [#196](https://github.com/taskcluster/taskcluster-rfcs/pull/196)
* Proposed by: @ahal

# Summary

In addition to scheduling tasks, the Github service will be able to trigger
hooks that are specified in a project's `.taskcluster.yml` file.

## Motivation

This will give instance administrators a way to provide "shared tasks" that
specific projects can opt into using without needing to copy / paste a large
blob of JSON-e.

Specifically in the Firefox-CI instance, it will allow us to define a single
well tested and maintained Decision task as a hook. Hiding the complexity from
projects and ensuring all projects are using the most recent and up-to-date
features.

# Details

A new `hooks` key will be added to the [taskcluster-yml-v1] schema. This key is
a list, where each item is an object containing `hookId` and optionally
additional `context`. Hook ID corresponds to an existing hook in the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the word context here, but I couldn't think of anything better.

Taskcluster instance, and `context` is a JSON serializable object that gets
forwarded as part of the payload in the [triggerHook] API. Taskcluster Github
will always implicitly include the following in the payload:

- `event` - the raw Github event object
- `now` - the current time, as a string
- `taskcluster_root_url` - the root URL of the Taskcluster deployment
- `tasks_for` - the Github event type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume these would overwrite any version of them provided in the context? If so, these should probably be forbidden from being in the context in the first place to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this proposal anything in the context would be in a separate namespace. So in the JSON-e it would be possible to have:

${payload.tasks_for}  # implicit
${payload.context.tasks_for}  # project provided

I agree it would be confusing if a project did this, but given they don't overlap, I don't think we should prevent it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaaah, okay. Yep, makes sense to me!


For example:

```yml
version: 1
hooks:
- hookId: decision/taskgraph
context:
trustDomain: mozilla
```

In this example, Taskcluster Github would make a call like:

```
triggerHook("decision", "taskgraph", {

"context": {
"trustDomain": "mozilla"
},
"event": <github event object>
"now": <timestamp>,
"taskcluster_root_url": "https://tc.example.com",
"tasks_for": "github-push",
})
```

## Permissions

Taskcluster Github will create a `hooks` client with scopes limited to the role
assumed earlier on based on the repository and Github event type. The role for
the current repo and context must have sufficient scopes to trigger the hook.
This works similarly to how Taskcluster Github currently calls the `createTask`
endpoint.

## Error Handling

If a `hookId` references a hook that does not exist, Taskcluster Github
will skip that hook and comment on the commit or pull request with an error.

If the `triggerHook` call fails, i.e due to a failure rendering the hook's
JSON-e body or a scope error, then Taskcluster Github will skip that hook and
comment on the commit or pull request with details of the failure.

In both cases, Taskcluster Github will not abort and will proceed with firing any
remaining hooks or scheduling remaining tasks.

## Github Builds

The `triggerHook` API returns the hook task's `taskId` as part of the response.
Taskcluster Github will use this `taskId` to create a record in the Github
builds table. This will allow Taskcluster Github to add the task to Github's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For checks and statuses to work properly, github internal queue listener depends on the routing keys - route.checks/route.statuses.
Maybe this should be part of the context passed to the trigger, since .taskcluster.yml knows which type to use

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, maybe it could just add reporting to the payload. Maybe also policy? Not sure if that would be useful though..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we just ignore status as isn't it supposedly deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we should make checks default (iirc currently statuses are being used if reporting is not defined)
Would policy be useful for hook payload? I thought github service would still make that decision prior to trigger that hook or not.
But we can surely pass whole .taskcluster.yml source that was used as part of the trigger payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't immediately think of a reason we'd need policy, so let's forget about that.

I'm equally happy adding reporting to the context, or for our use case, just assuming that everything is using checks (which afaik it is). Wnat me to update the RFC to include reporting?

checks UI and keep it updated with status changes.

This logic already exists for tasks scheduled based on the `tasks` key, so the
only new piece is obtaining the `taskId` from the `triggerHook` response rather
than parsing it out of the task definition.

## Edge Cases

If the `hooks` and `tasks` keys are both present in the `.taskcluster.yml`, the
hooks will be processed first, followed by the tasks.

If the hook's JSON-e template renders to `null`, then the hook service doesn't
create a task. In this case Taskcluster Github does not log any errors nor add
any records to the Github builds table.

# Implementation

<Once the RFC is decided, these links will provide readers a way to track the
implementation through to completion, and to know if they are running a new
enough version to take advantage of this change. It's fine to update this
section using short PRs or pushing directly to master after the RFC is
decided>

* <link to tracker bug, issue, etc.>
* <...>
* Implemented in Taskcluster version ...

[taskcluster-yml-v1]: https://docs.taskcluster.net/docs/reference/integrations/github/taskcluster-yml-v1
[triggerHook]: https://docs.taskcluster.net/docs/reference/core/hooks/api#triggerHook
1 change: 1 addition & 0 deletions rfcs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,4 @@
| RFC#189 | [Batch APIs for task definition, status and index path](0189-batch-task-apis.md) |
| RFC#190 | [Queue change task / task group priority](0190-queue-change-task-run-priority.md) |
| RFC#191 | [Worker Manager launch configurations](0191-worker-manager-launch-configs.md) |
| RFC#196 | [Trigger Hooks from Github Service](0196-trigger-hooks-from-github-service.md) |