-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
Changes from all commits
e266898
dfa136e
44a11fd
a5df9cd
a7cfb0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,39 +6,86 @@ name: Validate Docs | |
|
|
||
| on: [pull_request] | ||
|
|
||
| # A new push supersedes the run before it. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| NODE_VERSION: 24 | ||
|
|
||
| jobs: | ||
| test: | ||
| name: Test on ${{ matrix.os }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| # Build processes work differently across operating systems, so the | ||
| # checks run on each one to make sure anyone can contribute to the | ||
| # docs. | ||
| os: [windows-latest, macOS-latest] | ||
| # Needs no dependencies, so it reports in seconds rather than queueing behind | ||
| # npm ci and lint. | ||
| 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm less sure about the case-sensitivity reasoning. I dropped Typecheck can't cover it either. Neither our Adding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped that paragraph from the description. Adding npm run build:preview to the Linux job is worth doing, and I'd rather open a follow-up than grow this one. |
||
| 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: 24 | ||
| node-version: ${{ env.NODE_VERSION }} | ||
| cache: npm | ||
| - name: 🕸️ Install Dependencies | ||
| run: npm ci | ||
| - 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 | ||
| # Lint and spell check changes should be pushed | ||
| # to the branch before the branch is merge eligible. | ||
| # | ||
| # The translation keys check should not produce any changes. | ||
|
|
||
| cross-platform: | ||
| name: Cross-platform on ${{ matrix.os }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| # Prettier rewrites line endings, so Lint is the one check whose | ||
| # verdict depends on the operating system. | ||
| os: [windows-latest, macOS-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: 🖌️ Lint | ||
| run: npm run lint | ||
| # Lint changes should be pushed to the branch before the branch is merge | ||
| # eligible. | ||
| - name: 🔍 Check Diff | ||
| run: git diff --exit-code | ||
| shell: bash | ||
|
|
||
| verify: | ||
| # The one required check on main and the feature branches, so the jobs | ||
| # above can change without touching the protection rules. Renaming this | ||
| # blocks merges until those 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,17 +15,14 @@ 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. | ||
| # The caller has to check out with `fetch-depth: 0`, which a composite | ||
| # action cannot require for itself. | ||
| - 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" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This action now depends on the caller setting 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| node <<'JS' | ||
| const { readFileSync } = require('fs'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a
concurrencygroup 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 inbuild.ymlas not consuming more runners than we need to.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, keyed on ${{ github.workflow }}-${{ github.ref }} rather than the ref alone, since groups are repo-wide and a nightly ja build is on the backlog here. Happy to match yours exactly if you prefer. a5df9cd