Support check enforcer with Github Actions

[benbp]

Pre-requisite for Azure/azure-sdk-tools#4991

This PR adds support for evaluating github actions checks from the workflow_run trigger. The check enforcer on trigger has to be specified as below in order for triggers to work when the source is Github:

name: Check Enforcer

on:
  ... other triggers ...
  workflow_run:
    types: [completed]
    workflows: ["<my workflow 1>", "<my workflow 2>"]

Check Enforcer as written only supported a single "approved" app that we evaluated for check success/fail. This change also adds support for evaluating multiple app targets, and handling cases where only one target is configured to trigger for a specific directory.

There is one limitation I haven't solved yet, which is an edge case we get when one app has a passing check suite, and the other app fails to create any check runs due to invalid yaml when evaluating if it should trigger. Currently check enforcer will stay pending, because it expects a succeeded check suite, but now that criteria can be fulfilled by other apps.

[konrad-jamrozik]

@benbp just for my understanding I want to ask about this:

Check Enforcer as written only supported a single "approved" app that we evaluated for check success/fail. This change also adds support for evaluating multiple app targets, and handling cases where only one target is configured to trigger for a specific directory.

From what I understood the Check Enforcer works by being triggered on "Check Suite Completed" event and then evaluating if all checks are green. If so, it goes into green state.

I believe the case here is that so far we were dealing with only one check suite, corresponding to the GitHub app for Azure DevOps pipelines. But now you added support also for the GitHub actions GitHub app, and its check suite. Its check suite will be triggered via the workflow_run.

The expected behavior is now like that:

1. if both check suites are present (for ADO pipelines GitHub app and GitHub actions GitHub app), both of them have to be green for Check Enforcer to be green;
2. if one check suite is present, it has to be green for Check Enforcer to be green;
   • yet unsupported corner case: one check suite is present, but Check Enforcer cannot determine if there is another check suite, due to e.g. invalid YAML. This will prevent Check Enforcer from becoming green;
3. if no check suites are present, Check Enforcer is not green and needs to be overridden manually.

Is my understanding correct?

[benbp]

1. if both check suites are present (for ADO pipelines GitHub app and GitHub actions GitHub app), both of them have to be green for Check Enforcer to be green;

If both check suites are present, any check suite with latest_check_runs_count greater than 0 has to be green. I discovered github automatically creates check suites for each app with check:write permissions regardless of whether they post any runs or not. (See link to doc to highlight).

if one check suite is present, it has to be green for Check Enforcer to be green;

Correct, though I'm realizing that the above logic may have another gap, so I think we need to set an explicit minimum check runs now. I will test this.

yet unsupported corner case: one check suite is present, but Check Enforcer cannot determine if there is another check suite, due to e.g. invalid YAML. This will prevent Check Enforcer from becoming green;

Two check suites are present, one has check runs, the other has no check runs due to invalid yaml. That will pass, but ideally it stays pending. In the case with a single registered app, it will stay pending.

[konrad-jamrozik]

@benbp

yet unsupported corner case: one check suite is present, but Check Enforcer cannot determine if there is another check suite, due to e.g. invalid YAML. This will prevent Check Enforcer from becoming green;

Two check suites are present, one has check runs, the other has no check runs due to invalid yaml. That will pass, but ideally it stays pending. In the case with a single registered app, it will stay pending.

Thanks, that clears things up for me!

I understand before it was like that:

Azure DevOps pipelines GitHub app has invalid YAML, in the sense it should have included one of the changed paths, but didn't (i.e. syntax is valid, just the file author forgot to add the right path (or is actually invalid syntax?)) --> its Check Suite ends up having zero check runs --> Check Enforcer stays in pending --> correct behavior

But after this PR gets merged it is going to be:

Azure DevOps pipelines GitHub app has invalid YAML --> its Check Suite ends up having zero check runs

but also

GitHub pipelines GitHub app is valid --> all its checks pass --> Check Enforcer sees that its Check Suite had at least 1 passing check, so it passes --> this is incorrect behavior, because we effectively ignored the fact that ADO yaml was missing the path to check (or had invalid YAML syntax?)

The difficulty here stems from the fact we don't know if ADO having zero checks means "Invalid YAML, need to fail" vs "Nothing to check for ADO here, just pass". I understand both scenarios are conceptually expected. Specifically, we want to sometimes allow ADO to not have any checks, relying only on GitHub actions checks.

Basically, we need to have some way to distinguish between

• "ADO check suite had zero runs because the YAML was missing some path it should have"
• vs "ADO check suite had zero runs because the YAML with paths to check had invalid syntax"
• vs "ADO check suite had zero runs, as designed"

Based on the highlight you provided seems to me we won't be able to do that - the check suite event will be always there (well, at least if the YAML has valid syntax but the path is missing; not sure about invalid YAML syntax). Hence in practice we will probably have to assume that if at least one of these two apps has at least 1 check, and all checks are green, then we are good. I think it ties to this comment you made:

Correct, though I'm realizing that the above logic may have another gap, so I think we need to set an explicit minimum check runs now. I will test this.

[benbp]

Azure DevOps pipelines GitHub app has invalid YAML, in the sense it should have included one of the changed paths, but didn't (i.e. syntax is valid, just the file author forgot to add the right path (or is actually invalid syntax?)) --> its Check Suite ends up having zero check runs --> Check Enforcer stays in pending --> correct behavior

@konrad-jamrozik the latter. We occasionally see invalid syntax in the pipeline yaml files. So the path trigger will be configured correctly, but devops won't be able to parse the yaml and trigger the workflow.

The difficulty here stems from the fact we don't know if ADO having zero checks means "Invalid YAML, need to fail" vs "Nothing to check for ADO here, just pass". I understand both scenarios are conceptually expected. Specifically, we want to sometimes allow ADO to not have any checks, relying only on GitHub actions checks.

Now that I'm typing this out, I think the best solution to this issue is just to have better CI around changes made to ADO yaml files. We can submit the pipeline yaml to the devops server side validation API as part of our Analyze step in CI. I don't know if github has an equivalent API or if we'd need to do our own validation with the yaml schema, which I don't think would be super hard (e.g. my editor already does it automatically).

[benbp]

Also, I verified we do successfully enforce "single check suite, zero check runs, don't pass" because of this line where I check successCount > 0 (and I added a test for it in the latest commit). This along with better CI for yaml validation will cover all gaps, I think.