-
Notifications
You must be signed in to change notification settings - Fork 50
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
ci: Use pull_request_target
with permissions check
#142
Conversation
.github/workflows/main.yaml
Outdated
@@ -1,24 +1,21 @@ | |||
name: unitary | |||
|
|||
on: ["push", "pull_request"] |
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 disable test on push
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 also don't think pull_request_target
tests the merge commit, which may be a problem. maybe we should run all three?
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've added push
to master and pull_request
, but then without the integration tests
pull_request_target
with permissions checkpull_request_target
with permissions check
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.
looks like it's working - https://github.com/DanielSchiavini/titanoboa/actions/runs/7903435212/job/21571324351?pr=3
since we jerry-rigged `pull_request_target` to run against the merge commit
pull_request_target
with permissions checkpull_request_target
with permissions check
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.
LGTM
What I did
Changed the pipeline to use the
pull_request_target
, so users with push access may run the pipeline using secrets.How I did it
By using a small github script that calls getCollaboratorPermissionLevel
How to verify it
Make a PR targetting this branch (pipeline) in my fork. The integration pipeline should raise an error.
Here is an example of a successful check targeting this branch.
Description for the changelog
N/A
Cute Animal Picture