Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
There was a problem hiding this comment.
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.
| - uses: ./.github/workflows/actions/check-admonitions | ||
|
|
||
| # These produce the same verdict on any operating system, so they run once. | ||
| checks: |
There was a problem hiding this comment.
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: 🔤 Spell Check | ||
| run: npm run spellcheck | ||
| - uses: ./.github/workflows/actions/check-admonitions | ||
| - uses: ./.github/workflows/actions/check-translations |
There was a problem hiding this comment.
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.
|
|
||
| # One stable name for branch protection, so the matrix can change freely. | ||
| verify: | ||
| # The required check on main and the feature branches. Renaming this |
There was a problem hiding this comment.
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!
| @@ -6,9 +6,45 @@ name: Validate Docs | |||
|
|
|||
| on: [pull_request] | |||
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
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:
npm ci, lint, tests and spellcheck before reporting, and it stops annotating every finding twice.Also:
cache: npmon both installing jobs,NODE_VERSIONdeclared once at the top, and the matrix job renamed fromtest, 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: 0on its checkout replaces the targetedgit fetchit 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?
Other information
This needs a branch protection change the moment it merges, or merges break.
mainandmajor-10.0currently requireTest on macOS-latestandTest on windows-latest. Those job names no longer exist, so both branches will be waiting on checks that never report.Switch both to requiring
Verifyalone. It fails if any of the three fail, and it keeps working if the matrix changes later, which is why it exists.translation/jphas no required checks, so nothing to do there.One ordering note: GitHub's UI only offers names it has seen, so
Verifybecomes selectable after this runs once. Worth having someone with admin ready before merging.The
Verifypattern is borrowed from ionic-framework'sverify-*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 requireVerifyinstead clears them.