Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(pr-test): use GitHub API to get changed files + diff #21305

Merged
merged 2 commits into from
Dec 8, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 29 additions & 47 deletions .github/workflows/pr-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,19 @@
name: PR Test

on:
# The reason for building once it's pushed to `main`, too, is an
# optimization. This way, when workflow is built on the `main` branch
# anything that's stored in the cache (e.g. node_modules) can be
# re-used by consecutive PRs in other branches.
# See https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache
# and https://git.luolix.topmunity/t/how-come-the-actions-cache-is-failing-so-much-often/196279/3
caugner marked this conversation as resolved.
Show resolved Hide resolved
push:
pull_request:
branches:
- main
pull_request:
paths:
- ".github/workflows/pr-test.yml"
- "files/en-us/**"
caugner marked this conversation as resolved.
Show resolved Hide resolved

jobs:
tests:
runs-on: ubuntu-latest
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}

steps:
- uses: actions/checkout@v3
Expand All @@ -35,32 +34,27 @@ jobs:
run: |
yarn --frozen-lockfile

- name: Config git core.quotePath
- name: Get changed files
run: |
# If you don't do this, the get-diff-action won't be able to
# notice files that contain non-ascii characters.
# I.e.
#
# ▶ git status
# Changes not staged for commit:
# ...
# modified: "files/en-us/glossary/b\303\251zier_curve/index.html"
#
# But after you set `core.quotePath` you get:
#
# ▶ git status
# Changes not staged for commit:
# ...
# modified: "files/en-us/glossary/bézier_curve/index.html"
#
# Now, this gets used by the `git diff ...` inside get-diff-action.
git config --global core.quotePath false

- uses: technote-space/get-diff-action@v6.1.1
id: git_diff_content
with:
PATTERNS: files/**/*.+(html|md)
SET_ENV_NAME: GIT_DIFF_CONTENT
# Use the GitHub API to get the list of changed files
# documenation: https://docs.github.com/rest/commits/commits#compare-two-commits
DIFF_DOCUMENTS=$(gh api repos/{owner}/{repo}/compare/${{ env.BASE_SHA }}...${{ env.HEAD_SHA }} \
--paginate \
--jq '.files | .[] | select(.status|IN("added", "modified", "renamed", "copied", "changed")) | .filename')

# filter out files that are not markdown files
GIT_DIFF_CONTENT=$(echo "${DIFF_DOCUMENTS}" | egrep -i "^files/.*\.(md)$" | xargs)
echo "GIT_DIFF_CONTENT=${GIT_DIFF_CONTENT}" >> $GITHUB_ENV

# filter out files that are not attachments
GIT_DIFF_FILES=$(echo "${DIFF_DOCUMENTS}" | egrep -i "^files/.*\.(png|jpeg|jpg|gif|svg|webp)$" | xargs)
caugner marked this conversation as resolved.
Show resolved Hide resolved
echo "GIT_DIFF_FILES=${GIT_DIFF_FILES}" >> $GITHUB_ENV

# filter out files that are not _redirects.txt
GIT_DIFF_REDIRECTS=$(echo "${DIFF_DOCUMENTS}" | egrep -i "^files/.*/_redirects\.txt$" | xargs)
echo "GIT_DIFF_REDIRECTS=${GIT_DIFF_REDIRECTS}" >> $GITHUB_ENV
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Build changed content
if: ${{ env.GIT_DIFF_CONTENT }}
Expand Down Expand Up @@ -111,11 +105,11 @@ jobs:
# Save the PR number into the build
echo ${{ github.event.number }} > build/NR

# 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.
git diff --diff-filter=ACMRT ${{ github.event.pull_request.base.sha }} ${{ github.sha }} > build/DIFF
wget https://github.com/${{ github.repository }}/compare/${{ env.BASE_SHA }}...${{ env.HEAD_SHA }}.diff -O build/DIFF

- name: Merge static assets with built documents
if: ${{ env.GIT_DIFF_CONTENT }}
Expand Down Expand Up @@ -143,12 +137,6 @@ jobs:
name: build
path: build/

- uses: technote-space/get-diff-action@v6.1.1
with:
PATTERNS: files/**/*.+(png|jpeg|jpg|gif|svg|webp)
ABSOLUTE: true
SET_ENV_NAME: GIT_DIFF_FILES

- name: Check changed files
if: ${{ env.GIT_DIFF_FILES }}
run: |
Expand All @@ -157,12 +145,6 @@ jobs:
export CONTENT_ROOT=$(pwd)/files
yarn filecheck ${{ env.GIT_DIFF_FILES }}

- uses: technote-space/get-diff-action@v6.1.1
id: git_diff_redirects
with:
PATTERNS: files/**/_redirects.txt
SET_ENV_NAME: GIT_DIFF_REDIRECTS

- name: Validate redirects
if: ${{ env.GIT_DIFF_REDIRECTS }}
run: |
Comment on lines 148 to 150
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confused here. We have redirect check workflow. So should we remove this part or remove that workflow?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other call is slightly different, but I was also wondering about this call. I'd usually suggest the separate call, unless there actually is a benefit of running it after a build

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's keep it for now. The "redirect check" workflow is more strict, so in theory it should always also fail if this non-strict call (which just loads the file) fails. But I think it's a separate conversion, outside of the scope of this PR. 🙂

Expand Down