[ci] PR Review Companion should be called iff there is diffed content from PR Test

[bsmth]

summary:

1. PR is opened with no changes to ./files directory
2. "PR Test" = completed with some steps skipped
3. PR review companion will fail

Current behavior:

.github/workflows/pr-review-companion.yml:

name: PR review companion

on:
  workflow_run:
    workflows: ["PR Test"]
    types:
      - completed
    # This will run even if steps are skipped (e.g. if no `./files/**/*` changed)

jobs:
  review:
    # ...

We call the PR review companion from the PR Test workflow if it passes:

.github/workflows/pr-test.yml:

name: PR Test

on:
  pull_request:
    branches:
      - main
    paths:
      - .nvmrc
      - ".github/workflows/pr-test.yml"
      - "files/**"

jobs:
  tests:
    steps:
      # ...

      # Steps skipped if no files changed
      - uses: actions/upload-artifact@v3
        if: ${{ env.GIT_DIFF_CONTENT }}
        env:
          BUILD_OUT_ROOT: /tmp/build
        with:
          name: build
          path: ${{ env.BUILD_OUT_ROOT }}

      # Etc. This will complete the job and run PR companion even if no files changed

proposal:

What we want to do is to call the PR companion only if there are changes in the files we care about:

.github/workflows/pr-review-companion.yml:

name: PR review companion

# We only want to run this when called from the PR Test workflow
on:
  workflow_call:

jobs:
  review:
    runs-on: ubuntu-latest

We call the PR companion from "PR Test" if we have changes in the files we care about:

.github/workflows/pr-test.yml:

name: PR Test

on:
  pull_request:
    # no changes ...

jobs:
  tests:
    steps:
      # business as usual ...

      # Last step, everything looking good so far
      - name: Check changed files
        if: ${{ env.GIT_DIFF_FILES }}
        env:
          CONTENT_ROOT: ${{ github.workspace }}/mdn/content/files
          CONTENT_TRANSLATED_ROOT: ${{ github.workspace }}/files
        working-directory: ${{ github.workspace }}/mdn/content
        run: |
          echo ${{ env.GIT_DIFF_FILES }}

          yarn filecheck ${{ env.GIT_DIFF_FILES }}

      # ADDITIONS:
      # This is the end of the current job, now we want to call the PR companion
      - name: Call PR companion
        if: ${{ env.GIT_DIFF_FILES }}
        uses: ./.github/workflows/pr-review-companion.yml

[yin1999]

We should also consider that the workflow triggered by pull_request target can not access secret.

If we use the pull_request_target instead. We should try to avoid using secret.GITHUB_TOKEN (which has the write permission, instead we could setup a read-only secret token for this) in the build job, and add a spread job to use the secrets and upload built asset. (maybe we could set the output of the build job and add a if condition for the second job. I've made a test in my fork:

• skip the second job if no document has been modified: https://github.com/yin1999/translated-content/actions/runs/5888708394
• run the second job if there is any document has been modified: https://github.com/yin1999/translated-content/actions/runs/5888772523

Append:

We could set read-only permission for the build job!

Implementation: #28617

[bsmth]

Thanks @yin1999 - I like the after-build job.

In your PR I think we can go the route of reusable workflows to keep the logic isolated and avoid huge yml files.

There's some examples here that we might want to consider: https://github.com/bsmth/reusable-workflow-example/blob/main/.github/workflows/upload-artifact.yml

Calling the workflow from another will keep the context of the "main" workflow: https://docs.github.com/en/actions/learn-github-actions/contexts#github-context

I agree we could use outputs or define a workflow-scoped boolean ('needs_preview' or something) and in the job that calls the review companion, we can check if it needs to be run or not like:

  call-workflow:
    needs: upload-artifact
    if: ${{ env.NEEDS_PREVIEW }}
    uses: ./.github/workflows/review_companion.yml

What do you think?

[yin1999]

In your PR I think we can go the route of reusable workflows to keep the logic isolated and avoid huge yml files.

There's some examples here that we might want to consider: https://github.com/bsmth/reusable-workflow-example/blob/main/.github/workflows/upload-artifact.yml

Calling the workflow from another will keep the context of the "main" workflow: https://docs.github.com/en/actions/learn-github-actions/contexts#github-context

I agree we could use outputs or define a workflow-scoped boolean ('needs_preview' or something) and in the job that calls the review companion, we can check if it needs to be run or not like:

  call-workflow:
    needs: upload-artifact
    if: ${{ env.NEEDS_PREVIEW }}
    uses: ./.github/workflows/review_companion.yml

What do you think?

Yes. I like this idea :)

[yin1999]

@bsmth This issue has been resolved by #30064 :)

[bsmth]

Super, thank you! closing as done now 🎉