From ec4a8a8be16ed5181b0b7dc5a621d1960fea59f5 Mon Sep 17 00:00:00 2001 From: per1234 Date: Sat, 24 Jul 2021 02:58:25 -0700 Subject: [PATCH 1/3] Use a variable to define error message prefix Whenever the bot needs to communicate to the user about a blocking issue with their pull request that they are able to resolve, a standardized prefix is added to the situation-specific error message (":x: **ERROR:**") to draw their attention to this information. This standardized text occurred multiple times in the workflow, which might lead to it becoming inconsistent over time, or just more work to improve the text. Use of an environment variable ensures that all uses of the prefix will be consistent and allows it to be edited once in a single place. --- .github/workflows/manage-prs.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/manage-prs.yml b/.github/workflows/manage-prs.yml index efa8f5ab..2f6dcaeb 100644 --- a/.github/workflows/manage-prs.yml +++ b/.github/workflows/manage-prs.yml @@ -6,6 +6,7 @@ env: # GitHub user names to request reviews from in cases where PRs can't be managed automatically. - per1234 CHECK_SUBMISSIONS_FAIL_FLAG_ARTIFACT: check-submissions-failed + ERROR_MESSAGE_PREFIX: ":x: **ERROR:** " on: # pull_request_target trigger is used instead of pull_request so the token will have the write permissions needed to @@ -205,7 +206,7 @@ jobs: | A problem was found with your submission ${{ matrix.submission.submissionURL }} - :x: **ERROR:** ${{ matrix.submission.error }} + ${{ env.ERROR_MESSAGE_PREFIX }}${{ matrix.submission.error }} - name: Set checks result to fail if error detected by submission parser if: matrix.submission.error != '' @@ -296,7 +297,7 @@ jobs: issue_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} body: | | - :x: **ERROR:** [Arduino Lint](https://github.com/arduino/arduino-lint) found errors with ${{ matrix.submission.submissionURL }}: + ${{ env.ERROR_MESSAGE_PREFIX }}[Arduino Lint](https://github.com/arduino/arduino-lint) found errors with ${{ matrix.submission.submissionURL }}: ``` ${{ steps.read-lint-report.outputs.text-report }} @@ -462,7 +463,7 @@ jobs: issue_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} body: | | - :x: **ERROR:** Your submission meets all requirements. However, the pull request could not be merged. + ${{ env.ERROR_MESSAGE_PREFIX }}Your submission meets all requirements. However, the pull request could not be merged. Please follow this guide to resolve a merge conflict: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/resolving-a-merge-conflict-on-github From c674fc0225664858abb6cc91591d357813cbcd74 Mon Sep 17 00:00:00 2001 From: per1234 Date: Sat, 24 Jul 2021 04:39:23 -0700 Subject: [PATCH 2/3] Add a dedicated "Manage PRs" workflow job for review request The workflow result might indicate either that the PR author could require assistance from a maintainer or that something is wrong with the system. In this case, the situation is brought to the attention of the maintainers by requesting a pull request review from them. Due to the need to avoid requesting review from a maintainer when they are the PR author (which is not allowed and thus would result in a spurious workflow failure), the code for requesting this review is not as trivial as might be expected. Previously, this code was duplicated at multiple places in the workflow, and would become more so as additional code is added. The workflow is made cleaner by moving that duplicated code to a single dedicated job, which is facilitated by the recent reworking of the workflow structure. This is a pure refactoring and should have no effect on the workflow behavior. --- .github/workflows/manage-prs.yml | 63 +++++++++++++------------------- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/.github/workflows/manage-prs.yml b/.github/workflows/manage-prs.yml index 2f6dcaeb..02d2ad6d 100644 --- a/.github/workflows/manage-prs.yml +++ b/.github/workflows/manage-prs.yml @@ -470,37 +470,12 @@ jobs: Once that is done, it will be merged automatically. - - name: Request a review in case assistance is required - if: contains(toJSON(env.MAINTAINERS), github.actor) != true # Don't attempt to request review from PR author. - uses: octokit/request-action@v2.x - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - route: POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers - owner: ${{ github.repository_owner }} - repo: ${{ github.event.repository.name }} - pull_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} - reviewers: ${{ env.MAINTAINERS }} - not-submission: needs: - parse if: needs.parse.outputs.type != 'submission' # These request types can't be automatically approved. runs-on: ubuntu-latest - steps: - - name: Request pull request review - if: contains(toJSON(env.MAINTAINERS), github.actor) != true - uses: octokit/request-action@v2.x - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - route: POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers - owner: ${{ github.repository_owner }} - repo: ${{ github.event.repository.name }} - pull_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} - reviewers: ${{ env.MAINTAINERS }} - - name: Comment on required review uses: octokit/request-action@v2.x env: @@ -543,18 +518,6 @@ jobs: labels: | - "status: maintenance required" - - name: Request pull request review - if: contains(toJSON(env.MAINTAINERS), github.actor) != true - uses: octokit/request-action@v2.x - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - route: POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers - owner: ${{ github.repository_owner }} - repo: ${{ github.event.repository.name }} - pull_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} - reviewers: ${{ env.MAINTAINERS }} - - name: Comment on unexpected failure uses: octokit/request-action@v2.x env: @@ -582,3 +545,29 @@ jobs: :warning::warning::warning::warning: SLACK_COLOR: danger MSG_MINIMAL: true + + request-review: + needs: + - merge-fail + - not-submission + - unexpected-fail + if: > + always() && + ( + needs.merge-fail.result != 'skipped' || + needs.not-submission.result != 'skipped' || + needs.unexpected-fail.result != 'skipped' + ) + runs-on: ubuntu-latest + steps: + - name: Request pull request review from maintainer + if: contains(toJSON(env.MAINTAINERS), github.actor) != true # Don't attempt to request review from PR author. + uses: octokit/request-action@v2.x + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + route: POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers + owner: ${{ github.repository_owner }} + repo: ${{ github.event.repository.name }} + pull_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} + reviewers: ${{ env.MAINTAINERS }} From f8385ae72be5f2887d786185063262ff0fb6f2df Mon Sep 17 00:00:00 2001 From: per1234 Date: Sat, 24 Jul 2021 04:46:16 -0700 Subject: [PATCH 3/3] Handle general problems with pull request When possible, if problems are detected in a pull request, the bot will attempt to guide the PR author through the process of making a valid submission, which should be handled in a completely automated fashion on our end. It has become clear that we need to prevent the removal of the final newline from `repositories.txt`. The existing system did not accomodate this requirement. Submissions are validated on a per-library basis, and the bot comments based on identifying which library the problem applies to. But this newline removal is not necessarily related to any specific item added to the list. So handling for general problems with a submission PR is needed, which is added here. Because the PR author is more likely to require assistance with resolving this sort of problem, PR review from a maintainer is requested. --- .github/workflows/manage-prs.yml | 38 +++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/.github/workflows/manage-prs.yml b/.github/workflows/manage-prs.yml index 02d2ad6d..bd383d14 100644 --- a/.github/workflows/manage-prs.yml +++ b/.github/workflows/manage-prs.yml @@ -93,6 +93,7 @@ jobs: outputs: type: ${{ steps.parse-request.outputs.type }} + error: ${{ steps.parse-request.outputs.error }} arduinoLintLibraryManagerSetting: ${{ steps.parse-request.outputs.arduinoLintLibraryManagerSetting }} submissions: ${{ steps.parse-request.outputs.submissions }} index-entry: ${{ steps.parse-request.outputs.index-entry }} @@ -133,6 +134,7 @@ jobs: # Due to limitations of the GitHub Actions workflow system, dedicated outputs must be created for use in # certain workflow fields. echo "::set-output name=type::$(echo "$REQUEST" | jq -r -c '.type')" + echo "::set-output name=error::$(echo "$REQUEST" | jq -r -c '.error')" echo "::set-output name=arduinoLintLibraryManagerSetting::$(echo "$REQUEST" | jq -r -c '.arduinoLintLibraryManagerSetting')" echo "::set-output name=submissions::$(echo "$REQUEST" | jq -c '.submissions')" echo "::set-output name=index-entry::$(echo "$REQUEST" | jq -r -c '.indexEntry')" @@ -156,6 +158,34 @@ jobs: labels: | - ${{ needs.parse.outputs.type }} + parse-fail: + needs: + - parse + if: needs.parse.outputs.error != '' + + runs-on: ubuntu-latest + steps: + - name: Comment on error detected while parsing PR + uses: octokit/request-action@v2.x + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + route: POST /repos/{owner}/{repo}/issues/{issue_number}/comments + owner: ${{ github.repository_owner }} + repo: ${{ github.event.repository.name }} + issue_number: ${{ github.event.pull_request.number }}${{ github.event.issue.number }} + body: | + | + Hi @${{ github.actor }} + A problem was found with your pull request: + + ${{ env.ERROR_MESSAGE_PREFIX }}${{ needs.parse.outputs.error }} + + Please resolve this error. The checks will automatically run again once that is done. + + More information: + https://github.com/${{ github.repository }}/blob/main/README.md#if-the-problem-is-with-the-pull-request + check-submissions: name: Check ${{ matrix.submission.submissionURL }} needs: @@ -473,7 +503,10 @@ jobs: not-submission: needs: - parse - if: needs.parse.outputs.type != 'submission' # These request types can't be automatically approved. + # These request types can't be automatically approved. + if: > + needs.parse.outputs.type != 'submission' && + needs.parse.outputs.type != 'invalid' runs-on: ubuntu-latest steps: - name: Comment on required review @@ -497,6 +530,7 @@ jobs: unexpected-fail: needs: # Run after all other jobs + - parse-fail - merge-fail - check-submissions-fail - label @@ -548,12 +582,14 @@ jobs: request-review: needs: + - parse-fail - merge-fail - not-submission - unexpected-fail if: > always() && ( + needs.parse-fail.result != 'skipped' || needs.merge-fail.result != 'skipped' || needs.not-submission.result != 'skipped' || needs.unexpected-fail.result != 'skipped'