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

Trivial change, test PR #347

Merged
merged 14 commits into from
Mar 4, 2024
Merged

Conversation

filchristou
Copy link
Contributor

No description provided.

@filchristou
Copy link
Contributor Author

@gdalle I am having some permission issue in the Graphs.jl repo.

The same workflow works for my fork Trivial change, test PR · filchristou/Graphs.jl@f0c892d · GitHub but doesn't in JuliaGraphs Trivial change, test PR · JuliaGraphs/Graphs.jl@f0c892d · GitHub.

If you go to the "Set up job" -> "GITHUB_TOKEN Permissions" I get "write" in my fork and "read" in JuliaGraphs

Did you specifically modify something because from what i read around the default permissions for public PR should be read/write https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token.

The specific error I get is "GraphQL: Resource not accessible by integration (addComment)" and there is some discussion here git - "Resource not accessible by integration" on github post /repos/{owner}/{repo}/actions/runners/registration-token API - Stack Overflow

@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

It's weird cause the actions indeed have read/write permissions in Graphs.jl as well as the organization in general. It's not the most secure setting but it should work

@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

I think this is the answer: cli/cli#8374 (comment)

I think that you are opening a PR from a fork and expecting the workflow on: pull_request to have write permissions but as far as I know that will never happen for security reasons.

Over in https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token you can see that "Maximum access for pull requests from public forked repositories" is always going to be read.

It happens cause you're on a fork

@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

write-all doesn't change anything, I've tried it

The issue is that you're doing it from a fork, and so the comment posting will never work. In this case the best we can do is use the option from BenchmarkCI to print the benchmarking results inside the workflow log, in addition to the PR comment (which will only work from branches, i.e. for PRs made by maintainers).

@filchristou filchristou added the to-benchmark Put this label to trigger benchmarking (experimental) label Feb 28, 2024
@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

it seems the workflow didn't even run on those latest commits

@filchristou filchristou added to-benchmark Put this label to trigger benchmarking (experimental) and removed to-benchmark Put this label to trigger benchmarking (experimental) labels Feb 28, 2024
@filchristou
Copy link
Contributor Author

yeah. sorry for the noise. I will be playing around for a while. I am not satisfied with the logging public PR benchmarks in the workflow so I am investigating other possibilities. Let me know if you have any ideas.

@filchristou filchristou added to-benchmark Put this label to trigger benchmarking (experimental) and removed to-benchmark Put this label to trigger benchmarking (experimental) labels Feb 28, 2024
@filchristou filchristou added to-benchmark Put this label to trigger benchmarking (experimental) and removed to-benchmark Put this label to trigger benchmarking (experimental) labels Feb 28, 2024
@filchristou filchristou added to-benchmark Put this label to trigger benchmarking (experimental) and removed to-benchmark Put this label to trigger benchmarking (experimental) labels Feb 28, 2024
@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

When a workflow is triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a public fork. For more information, see "Events that trigger workflows."

Might be worth a shot?

From https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

@filchristou filchristou removed the to-benchmark Put this label to trigger benchmarking (experimental) label Feb 28, 2024
@filchristou filchristou added the to-benchmark Put this label to trigger benchmarking (experimental) label Feb 28, 2024
@filchristou
Copy link
Contributor Author

probably it is worth a chance. Now I am considering a chain of workflows as described here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

why so complicated? I think pull_request_target is much easier

@filchristou
Copy link
Contributor Author

I don't get what's the practical difference between pull_request and pull_request_target. An activity type of the pull_request_target is opened which I guess is the default created PR ? Also the docs mention

Avoid using this event if you need to build or run code from the pull request.

Which is exactly what benchmarking does.
However, if I can't get the chained workflows to work I will start considering it.

@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

@filchristou filchristou added to-benchmark Put this label to trigger benchmarking (experimental) and removed to-benchmark Put this label to trigger benchmarking (experimental) labels Feb 28, 2024
@gdalle
Copy link
Member

gdalle commented Feb 28, 2024

On second thought I agree with you this is a bad security practice, let's not use pull_request_target

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

@filchristou
Copy link
Contributor Author

filchristou commented Feb 28, 2024

@gdalle Okey. it seems that the workflow_run thingy will only work if that file is in the "default branch". For Graphs.jl that is the master.

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

So either I need to start experimenting with the master or changing the default for a little while. What do you prefer ?

Good thing is that all these changes do not interacting with user code.

@gdalle
Copy link
Member

gdalle commented Feb 29, 2024

Let's discuss it tonight at the community call if you can attend?

@filchristou
Copy link
Contributor Author

okey. yes I will join

@filchristou filchristou merged commit 9004fc1 into JuliaGraphs:benchx Mar 4, 2024
1 check passed
filchristou added a commit that referenced this pull request Mar 15, 2024
* First try on benchmark CI (#315)

(WIP)
TODO: store results and PR label triggering

* Test consecutive PR (#346)

Merge all new advancements on the `benchx` branch in order to do a follow up consecutive PR to test github actions

* Trivial change, test PR (#347)

* Trivial change, test PR

* Add PR write permission

* Test full permissions

* Granular permissions

* try write-all

* try label event

* labeled in pull_request

* Store results as artifact and trigger a consecutive workflow_run

* Workflow chain almost complete (WIP)

* correct .benchmarkci path

* Add benchx support branch

* Just try pull_request_target instead

* Remove target

* Get rid of s to get the chain going

---------

Co-authored-by: Guillaume Dalle <[email protected]>

* Simplify for master merge

* Fixed formatting

* Integrate review comments

---------

Co-authored-by: Guillaume Dalle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-benchmark Put this label to trigger benchmarking (experimental)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants