Skip to content

chore(ci): split OS-independent checks into their own jobs #4734

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.

Sign up for GitHub

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

Open
thetaPC wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −18
Open

chore(ci): split OS-independent checks into their own jobs #4734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
70 changes: 58 additions & 12 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,45 @@ name: Validate Docs

on: [pull_request]

@ShaneK ShaneK Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth adding a concurrency group while you're restructuring this? It takes the workflow from 2 concurrent jobs to 5, two of them windows and macOS legs, so a second push to a PR stacks a lot more than it used to. Every multi-job workflow in ionic-framework has one, with the reasoning spelled out in build.yml as not consuming more runners than we need to.

concurrency:
  group: ${{ github.ref }}
  cancel-in-progress: true


env:
NODE_VERSION: 24

jobs:
test:
name: Test on ${{ matrix.os }}
# The result does not vary by operating system, so this runs once.
admonitions:
name: Check Admonitions
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
# The check diffs against the pull request base, so it needs history.
fetch-depth: 0
- uses: ./.github/workflows/actions/check-admonitions

# These produce the same verdict on any operating system, so they run once.
checks:

@ShaneK ShaneK Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm less sure about the case-sensitivity reasoning. I dropped import DocsCard from '@components/global/docsCard' into docs/index.mdx and typecheck, spellcheck, test and lint all pass quite happily. Build that same tree on a case-sensitive filesystem and it dies with Cannot find module '@components/global/docsCard'. So the bug is real, but nothing on this job goes looking for it, and there's no docusaurus build step anywhere in the workflow on any OS.

Typecheck can't cover it either. Neither our tsconfig.json nor @docusaurus/tsconfig sets forceConsistentCasingInFileNames, so it's been on by default the whole time and Windows and macOS were already catching cased imports under src. The 279 pages importing from @site/static/usage/ are the exposed ones, and include is ["src", "index.d.ts"], so tsc never sees any of them.

Adding npm run build:preview here is what would catch it. Happy for that to be a follow-up, but I think the description should drop that reasoning if it's not happening in this PR.

name: Checks
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- name: ⚙️ Use Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
node-version: ${{ env.NODE_VERSION }}
cache: npm
- name: 🕸️ Install Dependencies
run: npm ci
- name: Test
run: npm run test
- name: 🧩 Typecheck
run: npm run typecheck
- name: 🔤 Spell Check
run: npm run spellcheck

cross-platform:
name: Cross-platform on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
matrix:
Expand All @@ -22,23 +58,33 @@ jobs:
- name: ⚙️ Use Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
node-version: 24
node-version: ${{ env.NODE_VERSION }}
cache: npm
- name: 🕸️ Install Dependencies
run: npm ci
# Prettier rewrites line endings, so this is the check most likely to
# differ between operating systems.
- name: 🖌️ Lint
run: npm run lint
- name: Test
run: npm run test
- name: 🧩 Typecheck
run: npm run typecheck
- name: 🔤 Spell Check
run: npm run spellcheck
- uses: ./.github/workflows/actions/check-admonitions
- uses: ./.github/workflows/actions/check-translations

@ShaneK ShaneK Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lint's reason for staying in the matrix is clear, but this one doesn't get one and it's the other expensive step. Was that deliberate?

I did check whether the shared Check Diff was what pinned it here, and it isn't. Everything the translations check writes is either gitignored or untracked, and git diff --exit-code doesn't look at untracked files. So it could move over to the Linux job without dragging a guard along.

# Lint and spell check changes should be pushed
# to the branch before the branch is merge eligible.
# Lint changes should be pushed to the branch before the branch is merge
# eligible.
#
# The translation keys check should not produce any changes.
- name: 🔍 Check Diff
run: git diff --exit-code
shell: bash

# One stable name for branch protection, so the matrix can change freely.
verify:
# The required check on main and the feature branches. Renaming this

@ShaneK ShaneK Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: there are two comments making the same point on this job, one above verify: and one here, so maybe collapse them into one.

The admonitions and checks jobs have the same thing going on. The reason you give in the description for splitting admonitions out is the better one anyway, that it needs no dependencies so it stops queueing behind npm ci, lint and the rest before it can report. Up to you!

# blocks merges until the protection rules are updated to match.
name: Verify
if: ${{ always() }}
needs: [admonitions, checks, cross-platform]
runs-on: ubuntu-latest

steps:
- name: Check job status
if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}
run: exit 1
6 changes: 0 additions & 6 deletions .github/workflows/actions/check-admonitions/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,12 @@ runs:
#
# Only the files the pull request touches are checked, so an existing
# page is never anyone else's problem to fix.
#
# The event payload has no file list, so the changed files come from a
# diff. The checkout is shallow and the base commit is fetched here
# rather than through `fetch-depth` on the checkout, which would pull
# the full history for every step in the job just to serve this one.
- name: 🔎 Check Admonitions
shell: bash
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
CHANGED_FILES: ${{ runner.temp }}/changed-files.txt
run: |
git fetch --quiet --no-tags --depth=1 origin "$BASE_SHA"
git diff --name-only --diff-filter=ACMR "$BASE_SHA" HEAD > "$CHANGED_FILES"

@ShaneK ShaneK Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This action now depends on the caller setting fetch-depth: 0, which a composite action can't declare for itself. On a depth-1 clone this line gives fatal: bad object <sha> and nothing else, so someone adding a second caller or tidying the checkout later has nothing pointing them at the cause.

Could the requirement go in a comment here rather than only at the call site? The perf worry in the comment you removed doesn't really apply now either, the Check Admonitions job ran in 11s with full history.

node <<'JS'
const { readFileSync } = require('fs');
Expand Down