-
-
Notifications
You must be signed in to change notification settings - Fork 61.1k
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(workflow): post build result as comment #10618
Conversation
Co-authored-by: David Ordás <[email protected]>
d7177ab
to
2023f99
Compare
This does the same thing I was attempting to do in #10562 so that can likely be closed in favor of this as I had not got a complete solution yet. |
|
||
- name: 'Comment on PR' | ||
env: | ||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
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.
Is GITHUB_TOKEN
available to forked PR's? From the GitHub docs:
Anyone can fork a public repository, and then submit a pull request that proposes changes to the repository's GitHub Actions workflows. Although workflows from forks do not have access to sensitive data such as secrets, they can be an annoyance for maintainers if they are modified for abusive purposes.
Reference
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 since this does not run on: pull_request
but on another workflow completed, secrets might be available.
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.
Yup! Secrets should be available in comment-pr.yml
flow because that's internal for the repo and does not trigger through the fork. (It's triggered indirectly.)
Or at least, this is to the best of my knowledge. If you're up for it, you could try to do a faulty PR to my fork on purpose, and we'll see if the action works. 🤔 No problem if you don't have time.
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.
Yeah I can do this tonight (US time). Will post the results here.
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 have created a PR on your branch. It is "awaiting workflow approvals". Let me know if I can do anything else here to test this, happy to help!
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 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.
Awesome, LGTM!
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.
From what I can tell this seems like a cleaner approach than what I had in #10562. I like the approach of using the GitHub CLI here.
Ahh, sorry, I didn't notice your PR. Yours is actually susceptible to the same vulnerability that was in my first attempt at this. If I understand it correctly, having a workflow with a write-access token while doing an explicit checkout can open up problems. I'd recommend you read the following resource, it'll help explain why LuigiImVector and I opted for this two-step process: You can see the issue here in your PR already: Despite your branch not being merged yet, the proposed CI pipeline is already being used. If it were given the permission to do what it wanted, other PRs could then go to make malicious changes later. So instead on |
Really appreciate this explanation. I ran into the same issue on another project recently as well. This approach definitely seems like the proper one. |
here goes! |
Seems to be working well 💪 |
Has been very helpful already |
What does this PR do?
Improve repo
For resources
Description
This builds on #6914 by LuigiImVector, which was itself built on #5564 by me. I have preserved LuigiImVector's authorship, as the majority of the work is theirs, but they haven't been active on GitHub lately, so I thought it'd be best to open a new PR.
This takes that PR and makes the following changes:
push
from linter workflow, we only need it onpull-request
, there's no need forpush
. This also avoids the comment action firing twice.You can see more context on my changes in my review:
I can see that PR actually got the greenlight to merge, but everyone was too busy. If this gets approved, I'd be happy to test this right away on merge and monitor incoming PRs actively for a while. (Reference)
Checklist:
Follow-up
Demo
You can see here, first, the lint action failed, so the comment was posted and a label added. Then later the lint action passed, so the label was removed.
Related