-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Add a bot to comment on PRs when linter fails #26553
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
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.
This is great, thanks.
However, I am worried that adding a new comment each time the linter fails can make the discussion thread of the PR very crowded very quickly.
Maybe the bot should delete the previous comments whenever it adds a new comment to the discussion and also when the linter step is green.
WDYT?
id: source-run-info | ||
with: | ||
token: ${{ secrets.GITHUB_TOKEN }} | ||
sourceRunId: ${{ github.event.workflow_run.id }} |
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 this potiuk/get-workflow-origin@v1_5
trustable? I don't really know how the security model of the github API works but is it possible to give this step a token with a limited set of permissions just to retrieve the issue number and nothing in write mode?
In particular I am not sure how the abuse of ${{ secrets.GITHUB_TOKEN }}
would make it possible for a malicious actions/setup-python@v3
to upload malicious wheels to pypi or anaconda.org's nightly wheel repo one way or another.
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 we should explicitly configure the permissions granted to the actions in this workflow file:
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.
See also:
https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
this includes examples.
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.
token permissions are set, now this action only has read access to pull requests.
This now deletes all previous comments, and leaves a rolling comment at the end. Each push creates a new one and is always reflecting the latest state of the PR. |
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.
This looks great, thanks!
This now keeps the first comment and updates it. It also skips the details if the message is too long and posting fails. It's tested here: adrinjalali/gh-action-test#5 |
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
In #23362 we decided to have a bot to comment something helpful when linter fails, so that it'd enable us to have more complicated linters (such as adding
isort
) without burdening the contributors too much.This PR adds a bot which would comment PRs with relevant information. An example run can be seen here: adrinjalali/gh-action-test#3
Note that we can't really test this until we merge it. The action in a PR form a fork doesn't have permission to post a comment.