RFC 0196 - Trigger hooks from Github Service

[ahal]

Rendered

[lotas]

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

[ahal]

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

[ahal]

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

[lotas]

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.

[ahal]

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?

[lotas]

Thank you @ahal, that's a solid proposal.

As I understand we don't limit the number of hooks we could trigger here, same way as we didn't limit number of tasks created.
And as I understand a possible use case would be to create hooks dedicated to various events that would check the payload and decide which set of tasks needs to be created? (checking github event and action)
If that's the case, I wonder if we could also include filtering on the .taskcluster.yml side to fire some hooks only for specific github events?

But I guess this might not be the desired flow

[ahal]

So we need to filter on more than just the Github event type and action. Off the top of my head, we also filter on branch name and repo URL, but there are probably others.

Currently we use the JSON-e in the tasks key to do this filtering. We setup the logic such that an empty list is returned, and then TC Github just does nothing (no errors, no tasks). I think we'd want to do the same here, e.g do the filtering in the hook's JSON-e template.

From the docs:

If the task template renders to null, no task is created.

This gives us more control over where tasks can run.

I'm not opposed to TC Github also providing a way to do some basic filtering, I just don't think it would benefit our specific use case much. Unless it's trivial to do, I'd suggest leaving it out of this RFC to keep things simple. We can always implement later if needed.

[ahal]

Added a blurb to the "Edge Cases" section about what to do if the triggerHook call doesn't create any tasks.

[ahal]

I'm not opposed to TC Github also providing a way to do some basic filtering, I just don't think it would benefit our specific use case much. Unless it's trivial to do, I'd suggest leaving it out of this RFC to keep things simple. We can always implement later if needed.

Though thinking about it.. without any GH service side filtering, we would have a lot of triggerHook calls that render null tasks. One per Github check suite event. This isn't necessarily a problem, but it might be nice to reduce load on the hook service and make it easier to browse the hooks page in the UI.

I dunno, I could be persuaded either way.

[lotas]

Though thinking about it.. without any GH service side filtering, we would have a lot of triggerHook calls that render null tasks. One per Github check suite event. This isn't necessarily a problem, but it might be nice to reduce load on the hook service and make it easier to browse the hooks page in the UI.

Right, this might a good reason to not trigger a hook knowing it would be competing with valid tasks in the dedicated worker pools.

I quickly checked logs

jsonPayload.serviceContext.service="github"
jsonPayload.Fields.message=~"compiled with zero tasks"

Gives me ~9k for the last 2 weeks (most of which are web-platform-tests ~5k of those) So maybe in the grand scheme that is not too many tasks

[bhearsum]

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.

[ahal]

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.

[bhearsum]

aaaah, okay. Yep, makes sense to me!

[ahal]

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