-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Workflows] add a more secure way to run tests from a PR. #7969
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the security issue was, I probably missed a thread somewhere, but it's probably related to the gh
install. Too bad that we can't use it.
ref: ${{ github.event.inputs.branch }} | ||
repository: ${{ github.event.pull_request.head.repo.full_name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also work if the PR is created from a fork? I thought it would not, which is why I liked the gh
approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, yes. Cc: @glegendre01
Actually |
cc @DN6 |
|
||
- name: Run tests | ||
env: | ||
PY_TEST: ${{ github.event.inputs.test }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want to allow arbitrary string inputs. e.g. This string --collect-only; whoami
will run the command after ;
I suppose only users in the HF org can dispatch this workflow right? Just feels a bit risky. cc: @glegendre01 to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a validation step in the workflow which looks for things like that.
@DN6 addressed your comments. Let's wait for @glegendre01's comments on #7969 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small comment. LGTM 👍🏽
What does this PR do?
Apologies for not giving enough consideration to it before. So, with the help of @glegendre01, we devised a more secure way to achieve what we did in #7942.
We tested it on a dummy repo, too, and it works perfectly fine: https://github.com/huggingface/check-pr-test-workflows/actions/runs/9126813322/job/25095821575.
It also eliminates the
gh
installation. Shoutout to @glegendre01 for the massive help.