Skip to content

Conversation

@Joe-Downs
Copy link
Member

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]
(cherry picked from commit ef866a4)

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]>
(cherry picked from commit ef866a4)
@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@jsquyres
Copy link
Member

jsquyres commented Sep 11, 2022

See #10786 (comment) for why I am merging this PR even though the required "Git commit checker" CI test is not satisfied, and not even bothering to allow the OMPI AWS Jenkins CI tests to run before merging.

@jsquyres jsquyres merged commit fa738c5 into open-mpi:v5.0.x Sep 11, 2022
@rhc54
Copy link
Contributor

rhc54 commented Sep 11, 2022

Is it worth bringing any/all of these over to the PMIx and PRRTE repos?

@jsquyres
Copy link
Member

jsquyres commented Sep 11, 2022

You tell me. 😄 Here's what OMPI's github actions are currently doing:

  1. Git Commit Checks
    • This is basically doing all the same things we have been doing for forever, but doing them a bit "better" inside of a Github action. The definition of "better" here includes:
      • No need to setup an external web server to host the code
      • No need to setup extra authentication keys
      • More reliable than how we've done this before
      • Visible to anyone; you can see (and re-run, if necessary) the GitHub Actions
    • These tests are performed:
      • Check that email addresses are "good" (no "root", no "localhost", etc.)
      • Ensure that there is a "signed of by" message in all commits (except reverts)
      • On designated branches, ensure that there is a "cherry picked" message in the commits (unless overridden)
    • If any of these tests fail, fail the CI test. The thing that @Joe-Downs just added to this is that now if any of these tests fail, a comment is added to the PR explaining what commit failed and why. See git-commit-checks: comment on PR with error(s) #10786 for a screenshot showing a PR comment that lists the "bad" commits and why they are "bad".
  2. Labeler
    • This one is new. For PRs, it looks at the destination branch and sees if there is a github label named "Target: [branchname]". If so, it applies that label to the PR.
    • Joe is working on doing the same thing for milestones, too (i.e., if there's a github milestone named "[branchname]", then apply it to the PR. That doesn't exist yet, but will probably be added to the labeler soon(ish).

@rhc54
Copy link
Contributor

rhc54 commented Sep 11, 2022

Sounds like these would be good to have - would someone be able to port them? My brain is mush (okay, more mushy than normal) due to a pain injection gone bad.

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.

4 participants