git-commit-checks: comment on PR with error(s)

[Joe-Downs]

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().

Signed-off-by: Joe Downs joe@dwns.dev

---

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

[ompiteam-bot]

Can one of the admins verify this patch?

[jsquyres]

ok to test

[Joe-Downs]

Before it's merged, should this be pull_request_target for the run event? Or is it okay for the commit checker to not run on PRs with merge conflicts?

[jsquyres]

Per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target and https://securitylab.github.com/research/github-actions-preventing-pwn-requests/, I think we're ok to change pull_request to pull_request_target, because we're not compiling or running any of the PR code in the github action.

...unless the PR is changing the github action itself. But I think that's always a problem for pull_request_target.

[Joe-Downs]

Okay, the event trigger has been changed to pull_request_target. As far as the functionality of this action, it should be ready to merge.

[jsquyres]

@Joe-Downs Interesting. Why isn't this action being run on this PR? "Git commit checker" is still stuck at "Expected" -- it hasn't been run.

[Joe-Downs]

Hmm. That's strange.

This stackoverflow discussion seems to suggest it's just a thing that happens with Actions sometimes, and re-triggering them should fix it.

This GH community discussion mentions renaming action names different than what's in the branch protection rules can cause this. We didn't rename the action, but maybe it's because of the trigger change?

I'll look into this and see if I can find a definitive answer

[jsquyres]

I'm going to guess that it's some kind of weirdness with changing the trigger of an existing Action. E.g., it's getting treated as a "new" action (and therefore it won't run until it exists in the base)...? Let me know what you find.

[jsquyres]

@Joe-Downs A further thought occurs to me: we have the "Git commit checker" CI test marked as "required" here in the main Open MPI repo. Does changing the trigger from pull_request to pull_request_target mess with this in any way? My thinking is that if this PR doesn't run the "Git commit checker" Action because the trigger changed (and therefore it's being treated as a "new" Action), does that mean that the required "Git commit checker" Action will get confused because it is looking for a "Git commit checker" on the pull_request trigger, not the pull_request_target trigger?

Also, what does it mean for all the PRs that are already open but not yet merged?

Can you do some tests on your own fork to figure out what will happen? And also figure out what we need to do here on the main Open MPI repo to bring this CI test change in with minimal disruption to already-existing PRs?

[Joe-Downs]

I don't think it's related to any branch protection rules. I had the same Expected — Waiting for status to be reported message on the PR in my fork and I don't have any branch protection rules set up.

I think your initial thought of it being related to changing triggers is correct. It appears that GitHub treats it as a new action. I force pushed the old main (from before I merged the PR in my fork) and made a new PR, with the one, final commit (515e9aa) and made a new PR. With this, GitHub doesn't even run (or attempt to run) the Commit Checker. Which leads me to believe it's an all-new action in GitHub's eyes, like how we saw the label bot never run before actually existing in the base branch.

Additionally, last night, I merged the original PR in my fork, and did some testing with the new Commit Checker code in main. Now, the action does run and on pull_request_target but it gives me some errors when trying to find the PR number from GITHUB_REF:

File "/home/runner/work/ompi/ompi/.github/workflows/git-commit-checks.py", line 410, in <module>
      main()
File "/home/runner/work/ompi/ompi/.github/workflows/git-commit-checks.py", line 392, in main
      pr = get_github_pr()
File "/home/runner/work/ompi/ompi/.github/workflows/git-commit-checks.py", line 357, in get_github_pr
      pr_num = int(match.group(1))
AttributeError: 'NoneType' object has no attribute 'group'

The offending code:

 def get_github_pr():
    g      = Github(GITHUB_TOKEN)
    repo   = g.get_repo(GITHUB_REPOSITORY)

    # Extract the PR number from GITHUB_REF
    match  = re.search("/(\d+)/", GITHUB_REF)
    pr_num = int(match.group(1))
    pr     = repo.get_pull(pr_num)

    return pr

I tried some debugging, by adding a print statement to see what GITHUB_REF is, but the changes I made to the Python script would not reflect in the action run. Running only what was already in main, I assume. That's about as far as I got last night. I'll have to look into the documentation more. I guess pull_request_target changes the environment variables?

[jsquyres]

@Joe-Downs A thought: can you have this PR set this hook to run on both pull_request and pull_request_target triggers? That would satisfy the CI requirements for this PR. Then we have a follow on PR that removes the pull_request trigger, but since the pull_request_target-triggered Action now exists on main, it'll still be able to satisfy the "required" attribute.

Can you try this on your fork to see if this two-step scheme works?

[jsquyres]

@Joe-Downs and I worked on this today (sorry for all the push notifications!). We did all the testing on a fork, and think that the code is now correct. To make comments on a PR, we needed to change the Github Action trigger from pull_request to pull_request_target, so Github won't actually run it on this PR (since it's a "new" Action). Therefore, we won't be able to satisfy the "Required" CI action on this one PR. Once this PR is merged, it will be fine for all existing and future Open MPI PRs -- it will use the new code from this PR.

Bottom line: I'm going to merge this PR even though we haven't satisfied the "Required" "Git commit checker" test. But it will be fine for all existing and future Open MPI PRs.

[Joe-Downs]

@jsquyres Welp, looks like we have a problem with the Commit Checker. In PR #10663, the bot ran, but had an error. Notably this:

stderr: 'fatal: ambiguous argument 'main..origin/typos/remaining': unknown revision or path not in the working tree.

My assumption is that our solution that worked for branches on one repo doesn't work across forks. I will do some testing to confirm what happened, and hopefully I'll be able to fix this.

In the meantime, I converted PRs #10660, #10661, and #10662 into drafts so they don't get merged before it's fixed.