Skip to content

Conversation

@Joe-Downs
Copy link
Member

@Joe-Downs Joe-Downs commented Sep 10, 2022

This is a re-submission of PR #10632 (which was reverted due to errors).

This adds comments onto the pull request containing what caused the commit checks to fail, if any, and suggests fixes to the user. If no errors are raised, no comment is made.

GitHub says there's a limit of 65536 characters on comments. If the bot's comment is over that limit, it will truncate the comment to fit, and add a message explaining where the remaining errors can be found. Unfortunately, the GitHub API doesn't seem to provide a job's unique ID for linking to a job run (this is different than an action run: ".../runs/..." vs ".../actions/runs/...", respectively), so we can't directly link to the error messages printed to the console. Additionally, to create this link, two new environment variables are used: GITHUB_RUN_ID and GITHUB_SERVER_URL.

Because we need the PR object twice, check_github_pr_description() was also changed to have the PR object passed into it; the PR object is gotten with a new function, get_github_pr().

The GitHub action configuration was changed to run on pull_request_target, instead of pull_request. Here are the differences:

  • The target repo (aka base repo) is cloned instead of the head repo.
    • This means we have to add a git remote for the head repo and fetch it.
  • The action runs in the security context of the target repo,instead of the head repo.
    • This allows us to post a comment on the pull request.
  • This action runs even if there are merge conflicts on the PR.

Similar to 5bf0b02, we restrict permissions of the action for pull_request_target.

Signed-off-by: Joe Downs [email protected]


Here's an example comment that this Github Action PR change will emit when there are problems with commits:
image

This adds comments onto the pull request containing what caused the commit
checks to fail, if any, and suggests fixes to the user. If no errors are raised,
no comment is made.

GitHub says there's a limit of 65536 characters on comments. If the bot's
comment is over that limit, it will truncate the comment to fit, and add a
message explaining where the remaining errors can be found. Unfortunately, the
GitHub API doesn't seem to provide a job's unique ID for linking to a job
run (this is different than an action run: ".../runs/..." vs
".../actions/runs/...", respectively), so we can't directly link to the error
messages printed to the console. Additionally, to create this link, two new
environment variables are used: GITHUB_RUN_ID and GITHUB_SERVER_URL.

Because we need the PR object twice, check_github_pr_description() was also
changed to have the PR object passed into it; the PR object is gotten with a new
function, get_github_pr().

The GitHub action configuration was changed to run on pull_request_target,
instead of pull_request. Here are the differences:

* The target repo (aka base repo) is cloned instead of the head repo.
  * This means we have to add a git remote for the head repo and fetch it.
* The action runs in the security context of the target repo,instead of the
  head repo.
  * This allows us to post a comment on the pull request.
* This action runs even if there are merge conflicts on the PR.

Similar to 5bf0b02, we restrict permissions of the action for
pull_request_target.

Signed-off-by: Joe Downs <[email protected]>
@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@jsquyres
Copy link
Member

Per discussion on #10632, we figured out that since this PR is changing the GitHub Action Trigger from pull_request to pull_request_target, GitHub will not run this Action on this PR. This is an intentional security feature: GitHub will not run PR Actions of a given Trigger type if they do not exist on the PR's base branch.

It is worth noting that this repo is configured to require that a CI test named "Git commit checker" pass. But per above, this GitHub Action will not be run on this PR. Hence, once this PR passes the other CI tests (which aren't even testing the content of this PR at all), I'll override the need for the required "Git commit checker" CI test and merge this PR. This is a one-time / chicken-and-egg override that needs to be done just to get the trigger change on to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants