Skip to content

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

Open
thetaPC wants to merge 1 commit into
mainfrom
ci-checks
Open

thetaPC wants to merge 1 commit into
mainfrom
ci-checks

Conversation

@thetaPC

@thetaPC thetaPC commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Issue URL: N/A

What is the current behavior?

One matrix job runs every check on Windows and macOS. Several of those read files and report a verdict that cannot vary by operating system, so they run twice for the same answer.

From a recent run, Windows / macOS: Spell Check 27s / 13s, Typecheck 4s / 2s, Test 2s / 1s. macOS bills at 10x Linux, Windows at 2x.

There is also no Linux runner anywhere, while Vercel builds on Linux. Both runners have case-insensitive filesystems, so an import with the wrong casing passes CI and fails production.

What is the new behavior?

Four jobs:

  • Check Admonitions (Linux) — the composite action only. It needs no dependencies, so it no longer waits on npm ci, lint, tests and spellcheck before reporting, and it stops annotating every finding twice.
  • Checks (Linux) — Test, Typecheck, Spell Check
  • Cross-platform on {os} (Windows, macOS) — Lint, check-translations, Check Diff
  • Verify — aggregates the three

Also: cache: npm on both installing jobs, NODE_VERSION declared once at the top, and the matrix job renamed from test, which no longer ran any.

Lint stays in the matrix deliberately. Prettier rewrites line endings and Check Diff catches it, so it is the one check that genuinely differs by OS. It is also the most expensive step at 59s / 32s.

Now that the admonition check has its own job, fetch-depth: 0 on its checkout replaces the targeted git fetch it used to do. That workaround existed to avoid pulling history for a job full of unrelated steps, which no longer applies.

Does this introduce a breaking change?

  • Yes
  • No

Other information

This needs a branch protection change the moment it merges, or merges break.

main and major-10.0 currently require Test on macOS-latest and Test on windows-latest. Those job names no longer exist, so both branches will be waiting on checks that never report.

Switch both to requiring Verify alone. It fails if any of the three fail, and it keeps working if the matrix changes later, which is why it exists. translation/jp has no required checks, so nothing to do there.

One ordering note: GitHub's UI only offers names it has seen, so Verify becomes selectable after this runs once. Worth having someone with admin ready before merging.

The Verify pattern is borrowed from ionic-framework's verify-* jobs, which exist for the same reason.

The two Test on ... checks will never report: this PR renames those jobs, and branch protection still requires the old names. Updating it to require Verify instead clears them.

@vercel

vercel Bot commented Sep 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
ionic-docs Ready Ready Preview Sep 14, 2026 10:49pm UTC

Request Review

@thetaPC
thetaPC marked this pull request as ready for review September 14, 2026 22:58
@thetaPC
thetaPC requested a review from a team as a code owner September 14, 2026 22:58
@thetaPC
thetaPC requested a review from ShaneK September 14, 2026 22:58

@ShaneK ShaneK left a comment

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.

Nice, this is a good split. The run dropped from 189s to 158s, and the admonition annotations show up at 11s now instead of waiting on Windows to finish, which is the bit I care about most.

One thing on the description though, the 10x/2x multipliers only kick in on private repos, so we're not actually paying for any of this. Doesn't make the change less worth doing, I'd just pitch it on the wall clock instead.

Left a few questions inline, the main one being whether the Linux job really buys us the case-sensitivity coverage. Happy for you to push back on any of them.

Comment thread .github/workflows/CI.yml
- uses: ./.github/workflows/actions/check-admonitions

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

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.

Comment thread .github/workflows/CI.yml
- name: 🔤 Spell Check
run: npm run spellcheck
- uses: ./.github/workflows/actions/check-admonitions
- uses: ./.github/workflows/actions/check-translations

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.

Comment thread .github/workflows/CI.yml

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

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!

Comment thread .github/workflows/CI.yml
@@ -6,9 +6,45 @@ name: Validate Docs

on: [pull_request]

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

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"

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants