[WIP] ci: attach commit status to PR for issue_comment triggers

[alexxa]

As demonstrated on PR #388 , PR comments starting with /retest and /test-functional without any other wording in front including spaces trigger Functional Tests workflow (see the screenshot attached).

By default, it's not visible within PR page. Workflows triggered by issue_comment run in the context of the default branch, not the PR. Because of this, GitHub does not automatically attach the workflow run to the PR's 'Checks' UI at the bottom of the page. One needs to go to 'Actions' to see it.

This PR adds the issue_comment run to show up at the bottom of a PR with a green checkmark/red X.

Yeah, it looks unusual to have a block of Python embedded in the middle of a Bash script inside a YAML file, but it feels like a reasonable workaround to communicate with the GitHub API from inside a container that lacks standard CLI networking tools like curl.

A kind reminder: just like the --xunit fix, this enhancement will only start functioning after it is merged into the main branch.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	94.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[owtaylor]

So the problem I had was pure PEBCAK - I subconciously interpreted the / in =~ ^/(test-functional|retest) as part of the regex syntax, not text to match, so tried just retest not /retest! Sorry for the confusion! But I do think that we want to not make someone dig through ever single test run to find the result.

One option that came to my attention is that maybe this could be triggered by adding a functional-tests label

The advantage of that is you can do:

  pull_request_target:
    types: [opened, synchronize, labeled]

Then check event.label.name as you do currently, or with an if:. Then everything runs in the pull request context, no special work is needed to retrieve the correct SHA, and it is properly integrated into the checks tab and status output.

If we can't do that - is it possible to use the "checks" API rather than the "status" API? This would probably result in a more consistent and easy to understand output.

Assuming that we need to use the API in some way, I'm not really a fan of extending the use of mixed/python shell like this - it's hard to read, and multiple levels of quoting and interpolation make it more error prone.

Let me throw out some options here - maybe one of them works and is appealing:

• apk add github-cli when using the testing-farm/cli image, then gh can be used as I suggested in the last PR
• pip install tft-cli could be used to add the testing-farm binary to the github-runner and avoid the container
• Use separate steps using actions/github-script - the node.js environment will override the container: from the job, and the environment set up there makes using the github API very concise.
• Use actions/checkout to checkout the source code (default branch, not PR branch) and use separate Python scripts

[owtaylor]

I think you can do core.setEnvironment('GIT_URL', gitUrl) end skip setting them in the subsequent env: block.

[owtaylor]

This wasn't quite my suggestion - it looks like you've implemented here that the functional-tests label as a gate on the other ways of triggering it on the pull request - so the tests aren't run initially even for a an authorized user, but once an authorized user has added the label the tests are run and any further updates to the PR cause the tests to run..

(Does allow an external contributor to create a trojan-horse PR that is initially fine, but on update does whatever we are trying to avoid happening by requiring approval?)

What I was suggesting is more hacky, but also simplifies things more radically. I was suggesting that adding the functional-test labels could be a replacement for /retest / /functional-tests. To allow repeated triggering, the action would immediately remove the label again. (The automatic triggering for authorized users would still be there.

This could allow completely dropping the issue_comment path, and the complexities it causes. It's not very discoverable, but neither is adding magic comments.