Streamline testing with all secrets

[nineinchnick]

Description

Streamline running tests with all secrets for approved commits.

This is based on https://github.com/imjohnbo/ok-to-test

The suggested workflow is:

• a contributor opens up a PR
• a regular PR workflow runs, without secrets
• a maintainer (anyone with repo write access) reviews the PR and decides it's:
  • safe (no changes that would leak secrets)
  • and requires running all tests
• a maintainer adds a comment like this: /test-with-secrets sha=<last-commit-sha>
• an additional workflow, with secrets, is started - will show up as test-with-secrets-command in the actions tab
• when the workflow is done, it adds its conclusion as an additional check in the PR

Implementation details - why this is secure:

• there's a workflow triggered on all PR comments, that verifies the person issuing the /test-with-secrets command has write permissions; this workflow needs to have elevated permissions on its own and should authenticate as an app; it needs the app to be installed in the repo and APP_ID and APP_PRIVATE_KEY secrets to be defined
• when the command is approved, it issues a repository_dispatch event that'll trigger the CI in the repo context with access to all secrets; the approved SHA is passed as a param to be checked out from the fork
• a regular workflow_dispatch can't be used because it can't be limited only to people with write access

Every PR commit needs to be checked, but the workflow runs in about 5 seconds per comment and here's the avg and max per day for the last few months:

select avg(num) as avg_num, max(num) as max_num from (
  select date_trunc('day', created_at), count(*) as num
  from issue_comments
  where created_at > current_date - interval '3' month
  group by 1
  order by 1
) a;

gives:

      avg_num      | max_num 
-------------------+---------
 46.20652173913044 |     100 
(1 row)

Is this change a fix, improvement, new feature, refactoring, or other? ci

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific) ci

How would you describe this change to a non-technical end user or system administrator? n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed. ( ) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. ( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required. ( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[nineinchnick]

I tested this on my fork:

• this PR: https://github.com/nineinchnick/trino/pull/8 that triggered this run: https://github.com/nineinchnick/trino/runs/6830890502?check_suite_focus=true - it failed because it used secrets that are invalid, but are defined
• using this app: https://github.com/apps/trino-comment-watcher - it doesn't really do anything, just defines a private key and can be installed in a repo with appropriate permissions, as defined in https://github.com/imjohnbo/ok-to-test/blob/master/app.yml#L11

[kokosing]

@ppalucha Do you want to review this?

[nineinchnick]

@findepi @alexjo2144 @ebyhr this is now ready. I finally tested it and made sure all previous comments are addressed.

This is the PR when I was doing all tests: https://github.com/nineinchnick/trino/pull/11 To summarize:

• if the additional workflow fails, it adds a failed check (test with secrets) to the PR, visible in the checks tab: @findepi I know you don't like how it shows up between the Pull Request Labeler and ci, but I don't have ideas how to improve this - there's no workflow that owns this check that would add a header.
• if the workflow fails, it also adds a comment in the original PR linking to it:
• if a maintainer makes a typo in the command name, it won't get any reactions
• if a maintainer makes a typo in the argument (sha=) or uses an invalid commit sha (anything else than the current head of the branch) the workflow will fail saying it expected a different commit; I guess the same would happen if someone pushes another commit before the run starts

To prepare this for merging, any maintainer has to:

• move https://github.com/nineinchnick/github-actions to the trinodb org
• help me transfer https://github.com/apps/trino-comment-watcher to the trinodb org, regenerate the private key and add it to this repo as a secret

Then I could update this PR with all the final SHAs

[sopel39]

@nineinchnick what this is blocked on?

cc @trinodb/maintainers This is a pre-prerequisite for triggering benchmarks from PRs

[nineinchnick]

@sopel39 it has been reviewed and it's only waiting for final approval. @hashhar was supposed to handle this, as it requires some manual actions during/after merging.

[sopel39]

@hashhar could you please take a look again at this PR?

[przemekak]

@hashhar Do you know when it can land?

[hashhar]

@sopel39 The "code review" here is done. It needs one of @martint / @electrum / @dain / @findepi to install the GitHub App into trinodb org and add some secrets.

[przemekak]

@nineinchnick Could you please address conflicts? @hashhar @findepi Can you approve it?

[nineinchnick]

I can't resolve conflicts until https://github.com/trinodb/github-actions/pull/15 gets merged