From 01127e2f3e5325e9982de058a495b3e9a3155d27 Mon Sep 17 00:00:00 2001 From: A1lo Date: Fri, 8 Dec 2023 22:35:47 +0800 Subject: [PATCH] refactor(ci/pr-review): use `workflow_call` instead of `workflow_run` event (#16891) --- .github/workflows/pr-review-companion.yml | 51 ++----------- .github/workflows/pr-test.yml | 88 +++++++++++++---------- 2 files changed, 57 insertions(+), 82 deletions(-) diff --git a/.github/workflows/pr-review-companion.yml b/.github/workflows/pr-review-companion.yml index 805f1911fefb4a..31bd00c63f36fa 100644 --- a/.github/workflows/pr-review-companion.yml +++ b/.github/workflows/pr-review-companion.yml @@ -2,65 +2,24 @@ # https://github.com/mdn/content/blob/main/.github/workflows/pr-review-companion.yml # with absolutely minor differences. -# Things to do and run after a "PR Test" workflow has finished successfully. +# Things to do and run after a "tests" job in "PR Test" workflow has completed successfully. # Note, as of right now, this workflow does a bunch of things. It might be # worth considering to break it up so there's a dedicated post-PR # workflow just to posting PR comments about flaws, for example. name: PR review companion -on: - workflow_run: - workflows: ["PR Test"] - types: - - completed +on: workflow_call jobs: review: runs-on: ubuntu-latest - - if: > - ${{ github.repository == 'mdn/translated-content' && - github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.conclusion == 'success' }} - steps: - name: "Download artifact" - uses: actions/github-script@v6 + uses: actions/download-artifact@v3 with: - script: | - var artifacts = await github.rest.actions.listWorkflowRunArtifacts({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id: ${{github.event.workflow_run.id }}, - }); - var matchArtifacts = artifacts.data.artifacts.filter((artifact) => { - return artifact.name == "build" - }); - if (matchArtifacts.length === 0) { - console.warn( - 'No artifacts to download probably just means nothing ' + - 'was built in the "PR test" workflow. That\'s OK. ' + - 'This is actually not a genuine CI error.' - ); - throw new Error( - 'No matched build artifacts. ' + - 'Perhaps nothing built in the "PR test" workflow' - ); - } - var matchArtifact = matchArtifacts[0]; - var download = await github.rest.actions.downloadArtifact({ - owner: context.repo.owner, - repo: context.repo.repo, - artifact_id: matchArtifact.id, - archive_format: 'zip', - }); - var fs = require('fs'); - fs.writeFileSync('${{github.workspace}}/build.zip', Buffer.from(download.data)); - - - name: Unzip what was downloaded - run: | - 7z x build.zip -obuild -bb1 + name: build + path: build - uses: actions/checkout@v4 with: diff --git a/.github/workflows/pr-test.yml b/.github/workflows/pr-test.yml index 436097ca663866..77f49f18e863a9 100644 --- a/.github/workflows/pr-test.yml +++ b/.github/workflows/pr-test.yml @@ -7,19 +7,24 @@ name: PR Test on: - pull_request: + # The `GITHUB_TOKEN` in workflows triggered by the `pull_request_target` event + # is granted read/write repository access. + # Please pay attention to limit the permissions of each job! + # https://docs.github.com/actions/using-jobs/assigning-permissions-to-jobs + pull_request_target: branches: - main - paths: - - .nvmrc - - ".github/workflows/pr-test.yml" - - "files/**" jobs: tests: # do not run on PRs in forks if: github.repository == 'mdn/translated-content' runs-on: ubuntu-latest + # Set the permissions to `read-all`, preventing the workflow from + # any accidental write access to the repository. + permissions: read-all + outputs: + has_assets: ${{ steps.build-content.outputs.has_assets }} env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} @@ -30,35 +35,13 @@ jobs: steps: - uses: actions/checkout@v4 - # Needed otherswise it will check out a merge commit which means - # you can't get a `git diff` for this PR compared to its base. - # with: - # ref: ${{ github.event.pull_request.head.sha }} - - - uses: actions/checkout@v4 - with: - repository: mdn/content - path: mdn/content - - - name: Setup Node.js environment - uses: actions/setup-node@v4 with: - node-version-file: ".nvmrc" - cache: "yarn" - cache-dependency-path: mdn/content/yarn.lock - - - name: Install all yarn packages - working-directory: ${{ github.workspace }}/mdn/content - run: | - yarn --frozen-lockfile - env: - # https://github.com/microsoft/vscode-ripgrep#github-api-limit-note - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + ref: "${{ env.HEAD_SHA }}" - name: Get changed files run: | # Use the GitHub API to get the list of changed files - # documenation: https://docs.github.com/rest/commits/commits#compare-two-commits + # documentation: https://docs.github.com/rest/commits/commits#compare-two-commits DIFF_DOCUMENTS=$(gh api repos/{owner}/{repo}/compare/${{ env.BASE_SHA }}...${{ env.HEAD_SHA }} \ --jq '.files | .[] | select(.status|IN("added", "modified", "renamed", "copied", "changed")) | .filename') @@ -73,7 +56,30 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - uses: actions/checkout@v4 + if: ${{ env.GIT_DIFF_CONTENT }} || ${{ env.GIT_DIFF_FILES }} + with: + repository: mdn/content + path: mdn/content + + - name: Setup Node.js environment + if: ${{ env.GIT_DIFF_CONTENT }} || ${{ env.GIT_DIFF_FILES }} + uses: actions/setup-node@v4 + with: + node-version-file: ".nvmrc" + cache: yarn + cache-dependency-path: mdn/content/yarn.lock + + - name: Install all yarn packages + if: ${{ env.GIT_DIFF_CONTENT }} || ${{ env.GIT_DIFF_FILES }} + working-directory: ${{ github.workspace }}/mdn/content + run: yarn --frozen-lockfile + env: + # https://github.com/microsoft/vscode-ripgrep#github-api-limit-note + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Build changed content + id: build-content if: ${{ env.GIT_DIFF_CONTENT }} env: CONTENT_ROOT: ${{ github.workspace }}/mdn/content/files @@ -81,7 +87,7 @@ jobs: # This is so that if there's a single 'unsafe_html' flaw, it # completely fails the build. - # But all other flaws should be 'warn' so that we can include + # But all other flaws should be 'warn', so that we can include # information about the flaws when we analyze the built PR. BUILD_FLAW_LEVELS: "unsafe_html: error, *:warn" @@ -94,7 +100,7 @@ jobs: BUILD_LIVE_SAMPLES_BASE_URL: https://live.mdnyalp.dev BUILD_LEGACY_LIVE_SAMPLES_BASE_URL: https://live-samples.mdn.allizom.net - # In these builds we never care for or need the ability to sign in. + # In these builds, we never care for or need the ability to sign in. # This environment variable will disable that functionality entirely. REACT_APP_DISABLE_AUTH: true @@ -112,21 +118,21 @@ jobs: # the BUILD_OUT_ROOT and CONTENT_ROOT env vars. node node_modules/@mdn/yari/build/cli.js ${{ env.GIT_DIFF_CONTENT }} - echo "Disk usage size of build/" + echo "Disk usage size of build" du -sh ${{ env.BUILD_OUT_ROOT }} # Save the PR number into the build echo ${{ github.event.number }} > ${{ env.BUILD_OUT_ROOT }}/NR - - name: Download the diff file - if: ${{ env.GIT_DIFF_CONTENT }} - run: | - # Save the raw diff blob and store that inside the ./build/ + # Download the raw diff blob and store that inside the build # directory. # The purpose of this is for the PR Review Companion to later # be able to use this raw diff file for the benefit of analyzing. wget https://github.com/${{ github.repository }}/compare/${{ env.BASE_SHA }}...${{ env.HEAD_SHA }}.diff -O ${{ env.BUILD_OUT_ROOT }}/DIFF + # Set the output variable so the next job could skip if there are no assets + echo "has_assets=true" >> "$GITHUB_OUTPUT" + - name: Merge static assets with built documents if: ${{ env.GIT_DIFF_CONTENT }} run: | @@ -151,3 +157,13 @@ jobs: echo ${{ env.GIT_DIFF_FILES }} yarn filecheck ${{ env.GIT_DIFF_FILES }} + + review: + needs: tests + if: ${{ needs.tests.outputs.has_assets }} + # write permissions are required to create a comment in the corresponding PR + permissions: write-all + uses: ./.github/workflows/pr-review-companion.yml + # inherit the secrets from the parent workflow + # https://docs.github.com/actions/using-workflows/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow + secrets: inherit