Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline testing with all secrets #12817

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Conversation

nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented Jun 13, 2022

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`)

@cla-bot cla-bot bot added the cla-signed label Jun 13, 2022
@nineinchnick
Copy link
Member Author

I tested this on my fork:

@kokosing
Copy link
Member

@ppalucha Do you want to review this?

@nineinchnick nineinchnick added the no-release-notes This pull request does not require release notes entry label Jul 18, 2022
@nineinchnick nineinchnick force-pushed the ok-to-test branch 6 times, most recently from 37e1879 to 7ed039a Compare August 1, 2022 15:34
@nineinchnick nineinchnick force-pushed the ok-to-test branch 5 times, most recently from fd4e839 to 0444e46 Compare August 4, 2022 09:15
@przemekak
Copy link
Member

@hashhar Do you know when it can land?

});
return result;
}
const { data: result } = await github.rest.checks.create({
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that there are checks that are created only once they finish? If that's the case then maybe it's worth creating them first, just to indicate that something is still in progress.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@hashhar
Copy link
Member

hashhar commented Nov 3, 2022

@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
Copy link
Member

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

@nineinchnick
Copy link
Member Author

I can't resolve conflicts until trinodb/github-actions#15 gets merged

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm merging this @nineinchnick - thanks for the work here.

FYI @trinodb/maintainers you can now try to "approve" a commit from a fork to run CI with secrets once you have verified it looks ok to test.

Also since I have limited availability please keep an eye open for some time if this runs into issues.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

@hashhar hashhar merged commit 93f4cfc into trinodb:master Nov 29, 2022
@github-actions github-actions bot added this to the 404 milestone Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

7 participants