From 4b0ce7e5e1a1b7429e22ea861d6d84ecb478fb90 Mon Sep 17 00:00:00 2001 From: Joe Downs Date: Mon, 1 Aug 2022 17:28:16 -0400 Subject: [PATCH] git-commit-checks: comment on PR with error(s) 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. This allows the action to be run in the context of the base of the PR, rather than in the context of the merge commit. Therefore, the action is run even if the PR has merge conflicts. Because of how the branch contexts are different between pull_request and pull_request_target, other parts of the Python script had to change to reflect these differences. Signed-off-by: Joe Downs --- .github/workflows/git-commit-checks.py | 102 ++++++++++++++++++------ .github/workflows/git-commit-checks.yml | 7 +- 2 files changed, 82 insertions(+), 27 deletions(-) diff --git a/.github/workflows/git-commit-checks.py b/.github/workflows/git-commit-checks.py index 4a9f8e50bfc..8acbd4f4c94 100755 --- a/.github/workflows/git-commit-checks.py +++ b/.github/workflows/git-commit-checks.py @@ -8,13 +8,15 @@ variables that are available in the Github Action environment. Specifically: * GITHUB_WORKSPACE: directory where the git clone is located -* GITHUB_SHA: the git commit SHA of the artificial Github PR test merge commit * GITHUB_BASE_REF: the git ref for the base branch +* GITHUB_HEAD_REF: the git commit ref of the head branch * GITHUB_TOKEN: token authorizing Github API usage * GITHUB_REPOSITORY: "org/repo" name of the Github repository of this PR * GITHUB_REF: string that includes this Github PR number +* GITHUB_RUN_ID: unique ID for each workflow run +* GITHUB_SERVER_URL: the URL of the GitHub server -This script tests each git commit between (and not including) GITHUB_SHA and +This script tests each git commit between (and not including) GITHUB_HEAD_REF and GITHUB_BASE_REF multiple ways: 1. Ensure that the committer and author do not match any bad patterns (e.g., @@ -53,19 +55,25 @@ NACP = "bot:notacherrypick" GITHUB_WORKSPACE = os.environ.get('GITHUB_WORKSPACE') -GITHUB_SHA = os.environ.get('GITHUB_SHA') GITHUB_BASE_REF = os.environ.get('GITHUB_BASE_REF') +GITHUB_HEAD_REF = os.environ.get('GITHUB_HEAD_REF') GITHUB_TOKEN = os.environ.get('GITHUB_TOKEN') GITHUB_REPOSITORY = os.environ.get('GITHUB_REPOSITORY') GITHUB_REF = os.environ.get('GITHUB_REF') +GITHUB_RUN_ID = os.environ.get('GITHUB_RUN_ID') +GITHUB_SERVER_URL = os.environ.get('GITHUB_SERVER_URL') +PR_NUM = os.environ.get('PR_NUM') # Sanity check if (GITHUB_WORKSPACE is None or - GITHUB_SHA is None or GITHUB_BASE_REF is None or + GITHUB_HEAD_REF is None or GITHUB_TOKEN is None or GITHUB_REPOSITORY is None or - GITHUB_REF is None): + GITHUB_REF is None or + GITHUB_RUN_ID is None or + GITHUB_SERVER_URL is None or + PR_NUM is None): print("Error: this script is designed to run as a Github Action") exit(1) @@ -85,6 +93,50 @@ def make_commit_message(repo, hash): #---------------------------------------------------------------------------- +""" +Iterate through the BAD results, collect the error messages, and send a nicely +formatted comment to the PR. + +For the structure of the results dictionary, see comment for print_results() +below. + +""" +def comment_on_pr(pr, results, repo): + # If there are no BAD results, just return without posting a comment to the + # GitHub PR. + if len(results[BAD]) == 0: + return + + comment = "Hello! The Git Commit Checker CI bot found a few problems with this PR:" + for hash, entry in results[BAD].items(): + comment += f"\n\n**{hash[:8]}: {make_commit_message(repo, hash)}**" + for check_name, message in entry.items(): + if message is not None: + comment += f"\n * *{check_name}: {message}*" + comment_footer = "\n\nPlease fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!" + + # GitHub says that 65536 characters is the limit of comment messages, so + # check if our comment is over that limit. If it is, truncate it to fit, and + # add a message explaining with a link to the full error list. + comment_char_limit = 65536 + if len(comment + comment_footer) >= comment_char_limit: + run_url = f"{GITHUB_SERVER_URL}/{GITHUB_REPOSITORY}/actions/runs/{GITHUB_RUN_ID}?check_suite_focus=true" + truncation_message = f"\n\n**Additional errors could not be shown...\n[Please click here for a full list of errors.]({run_url})**" + # Cut the comment down so we can get the comment itself, and the new + # message in. + comment = comment[:(comment_char_limit - len(comment_footer + truncation_message))] + # In case a newline gets split in half, remove the leftover '\' (if + # there is one). (This is purely an aesthetics choice). + comment = comment.rstrip("\\") + comment += truncation_message + + comment += comment_footer + pr.create_issue_comment(comment) + + return + +#---------------------------------------------------------------------------- + """ The results dictionary is in the following format: @@ -242,15 +294,15 @@ def _is_entirely_submodule_updates(repo, commit): #---------------------------------------------------------------------------- def check_all_commits(config, repo): - # Get a list of commits that we'll be examining. Use the progromatic form - # of "git log GITHUB_BASE_REF..GITHUB_SHA" (i.e., "git log ^GITHUB_BASE_REF - # GITHUB_SHA") to do the heavy lifting to find that set of commits. + # Get a list of commits that we'll be examining. Use the programmatic form + # of "git log GITHUB_BASE_REF..GITHUB_HEAD_REF" (i.e., "git log + # ^GITHUB_BASE_REF GITHUB_HEAD_REF") to do the heavy lifting to find that + # set of commits. Because we're using pull_request_target, GITHUB_BASE_REF + # is already checked out. GITHUB_HEAD_REF has never been checked out, so we + # specify "origin/{GITHUB_HEAD_REF}". git_cli = git.cmd.Git(GITHUB_WORKSPACE) - hashes = git_cli.log(f"--pretty=format:%h", f"origin/{GITHUB_BASE_REF}..{GITHUB_SHA}").splitlines() - - # The first entry in the list will be the artificial Github merge commit for - # this PR. We don't want to examine this commit. - del hashes[0] + hashes = git_cli.log(f"--pretty=format:%h", + f"{GITHUB_BASE_REF}..origin/{GITHUB_HEAD_REF}").splitlines() #------------------------------------------------------------------------ @@ -292,15 +344,7 @@ def check_all_commits(config, repo): If "bot:notacherrypick" is in the PR description, then disable the cherry-pick message requirement. """ -def check_github_pr_description(config): - 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) - +def check_github_pr_description(config, pr): if pr.body and NACP in pr.body: config['cherry pick required'] = False @@ -334,11 +378,17 @@ def load_config(): def main(): config = load_config() - check_github_pr_description(config) - repo = git.Repo(GITHUB_WORKSPACE) - results, hashes = check_all_commits(config, repo) - print_results(results, repo, hashes) + g = Github(GITHUB_TOKEN) + github_repo = g.get_repo(GITHUB_REPOSITORY) + pr_num = int(PR_NUM) + pr = github_repo.get_pull(pr_num) + check_github_pr_description(config, pr) + + local_repo = git.Repo(GITHUB_WORKSPACE) + results, hashes = check_all_commits(config, local_repo) + print_results(results, local_repo, hashes) + comment_on_pr(pr, results, local_repo) if len(results[BAD]) == 0: print("\nTest passed: everything was good!") diff --git a/.github/workflows/git-commit-checks.yml b/.github/workflows/git-commit-checks.yml index 4cf1036878e..6f1078f7ca6 100644 --- a/.github/workflows/git-commit-checks.yml +++ b/.github/workflows/git-commit-checks.yml @@ -1,7 +1,11 @@ name: GitHub Action CI +# We're using pull_request_target here instead of just pull_request so that the +# action runs in the context of the base of the pull request, rather than in the +# context of the merge commit. For more detail about the differences, see: +# https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target on: - pull_request: + pull_request_target: # We don't need this to be run on all types of PR behavior # See https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request types: @@ -32,3 +36,4 @@ jobs: run: $GITHUB_WORKSPACE/.github/workflows/git-commit-checks.py env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUM: ${{ github.event.number }}