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

[FEATURE] Enable secure external execution of tests #1853

Open
miguelgfierro opened this issue Nov 17, 2022 · 8 comments
Open

[FEATURE] Enable secure external execution of tests #1853

miguelgfierro opened this issue Nov 17, 2022 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@miguelgfierro
Copy link
Collaborator

Description

Since we removed the pull_request_target on GitHub (see #1840), external contributors won't trigger the tests. We need to think in ways of enabling this solution.

Expected behavior with the suggested feature

Other Comments

@miguelgfierro miguelgfierro added the enhancement New feature or request label Nov 17, 2022
@miguelgfierro
Copy link
Collaborator Author

miguelgfierro commented Nov 17, 2022

Ideas:

From Jianjie:
I found a framework on building these bots: probot
We could possibly set something up to listen on the github webhooks: pull_request_review_comment
And trigger a workflow: actions-create-workflow-dispatch

From Miguel:
Will it work if some of the core devs manually trigger the external action?

@miguelgfierro
Copy link
Collaborator Author

The current state is that when an external person does a PR, the admins need to press a button before the tests run, see https://github.com/recommenders-team/recommenders/actions/runs/6026551815/attempts/1

however, when this is pressed, then there is an authentication error with AzureML:

Error: Az CLI Login failed. Please check the credentials and make sure az is installed on the runner. For more information refer https://aka.ms/create-secrets-for-GitHub-workflows

See https://github.com/recommenders-team/recommenders/actions/runs/6026551815/job/16352596352?pr=1984

@miguelgfierro
Copy link
Collaborator Author

miguelgfierro commented Oct 9, 2023

Follow the idea of feathr. They use a label that can be only added by an admin, after it is done, the triggers are done also for people that come from a fork.

See https://github.com/feathr-ai/feathr/blob/main/.github/workflows/pull_request_push_test.yml#L68

Blair Chen is going to send us more info about it.

@miguelgfierro miguelgfierro self-assigned this Oct 16, 2023
@miguelgfierro
Copy link
Collaborator Author

hey @blrchen, one question, if seems that when we add the line if: github.event_name == 'pull_request' || (github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'safe to test')) in every PR, no matter whether it is from people inside the team or a person doing a fork, we need to add the label before the tests are executed.

Ideally what we would like is that the tests are executed automatically if one of the maintainers do a PR and that we need to add the label if the PR comes from a fork (an outsider). Do you know whether this is possible?

FYI @SimonYansenZhao @loomlike @anargyri

@blrchen
Copy link

blrchen commented Oct 18, 2023

We are a small team with only a few PR activities each week, so manually adding labels is manageable.

If you prefer automation, you might consider enabling a PR bot to assign labels automatically if a PR is submitted by someone from approved list.

@miguelgfierro
Copy link
Collaborator Author

thanks @blrchen. Do you have some code example of a similar bot?

@blrchen
Copy link

blrchen commented Oct 18, 2023

Hi @miguelgfierro ,

Unfortunately I don't have any samples. I've noticed that many repositories setup workflows to manage custom labels for issues and PRs. This could potentially be a solution.

@miguelgfierro
Copy link
Collaborator Author

miguelgfierro commented Oct 19, 2023

Things I've tried:

   if: contains(github.event.pull_request.labels.*.name, 'safe to test') || github.event.pull_request.head.repo.fork == 'false'`

   if: github.event.pull_request.head.repo.fork == 'false' || (github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'safe to test'))´

   if: (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == 'false') || (github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'safe to test'))

   if: (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.fork == 'false') || (github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'safe to test'))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants