mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
Nick Vigilante
5840ac5f6e
## Problem `Docs CI` fails on PRs that only touch binary assets under `docs/`. Example: [#25314](https://github.com/coder/coder/pull/25314), which swaps a single PNG and produces thousands of `MD010/no-hard-tabs`, `MD049/emphasis-style`, and `MD018/no-missing-space-atx` errors at columns like 16,285 of the image. ## Root cause The single `tj-actions/changed-files` step was doing two jobs at once: detecting which Markdown files changed (for `lint` and `fmt`), and gating whether the workflow had anything to do at all. Its `files` filter matched `docs/**` in addition to `**.md`, so any non-Markdown file under `docs/` (PNG, GIF, JPG, MP4, SVG) ended up in `all_changed_files` and was passed straight to `markdownlint-cli2`, which opened the file and parsed the binary bytes as Markdown. `markdownlint-cli2`'s own `ignores` setting is a discovery-time filter and does not gate files passed explicitly on the command line, so the filtering has to happen in the caller. ## Fix Adopt a per-tool convention: each downstream tool gets its own `changed-files` step scoped to the files that tool can process. For now that is a single `changed-md` step matching `**.md`, consumed by `lint` and `fmt`. A future tool (e.g. an image linter, video size check, or link checker) can be added purely additively, by appending another `changed-*` step and a step that consumes its output, without changing the existing filters. The workflow-level `on.push.paths` / `on.pull_request.paths` triggers stay broad (`docs/**`, `**.md`) so the workflow still runs on screenshot-only PRs; the per-tool filters decide which individual steps execute. On a screenshot-only PR the existing `if: steps.changed-md.outputs.any_changed == 'true'` guard skips `lint` and `fmt` cleanly. ## Verification - `actionlint .github/workflows/docs-ci.yaml` passes. - Reproduced the original failure locally: `pnpm exec markdownlint-cli2 docs/images/install/install_from_deployment.png` produces the same flood of violations seen in the failing CI run on #25314. - First revision of this PR (workflow with `**.md`-only filter, single `changed-files` step) was green on `Docs CI`; the current revision is structurally equivalent for the existing tools and just renames the step id and adds the per-tool comment. <details> <summary>Decision log</summary> - Considered adding `ignores` to `.markdownlint-cli2.jsonc` to skip non-Markdown files. Rejected: `markdownlint-cli2` treats `ignores` as a discovery-time glob filter and still lints files passed explicitly on the command line, so it would not have fixed the failure. - Considered narrowing the existing single `changed-files` step's `files` filter to `**.md` only. Rejected as the final shape: it solves the immediate bug but conflates "which Markdown files changed" with "should the workflow run at all", so adding a second tool with a different file set later (e.g. an image linter) would require contorting or duplicating that step. - Chose the per-tool-filter shape so adding a future tool is additive: one new `changed-*` step plus one new step that consumes its output, with no edits to existing steps. </details> ## Disclosure Opened on behalf of @nickvigilante by Coder Agents.
72 lines
2.6 KiB
YAML
72 lines
2.6 KiB
YAML
name: Docs CI
|
|
|
|
on:
|
|
push:
|
|
branches:
|
|
- main
|
|
# Self-reference removed from both push and pull_request: the `lint`
|
|
# and `fmt` steps gate on `tj-actions/changed-files` matching
|
|
# `docs/**` or `**.md`, so a workflow-only edit produced an empty
|
|
# run. `actionlint` and `make lint/actions` catch YAML problems
|
|
# before merge regardless. See DOCS-129.
|
|
paths:
|
|
- "docs/**"
|
|
- "**.md"
|
|
|
|
pull_request:
|
|
# Self-reference removed; see comment under `push:` above.
|
|
paths:
|
|
- "docs/**"
|
|
- "**.md"
|
|
|
|
permissions:
|
|
contents: read
|
|
|
|
jobs:
|
|
docs:
|
|
runs-on: ubuntu-latest
|
|
steps:
|
|
- name: Checkout
|
|
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
|
|
with:
|
|
persist-credentials: false
|
|
|
|
- name: Setup Node
|
|
uses: ./.github/actions/setup-node
|
|
|
|
# Per-tool changed-files filters. Each tool gets its own `changed-*`
|
|
# step scoped to the files it processes, keeping workflow-level `paths:`
|
|
# broad. Adding a tool (e.g. image linter) only needs a new step pair.
|
|
- uses: tj-actions/changed-files@22103cc46bda19c2b464ffe86db46df6922fd323 # v45.0.7
|
|
id: changed-md
|
|
with:
|
|
files: |
|
|
**.md
|
|
separator: ","
|
|
|
|
# Both downstream tools take file paths as argv. `tj-actions/changed-files`
|
|
# joins paths with `separator: ","`, which the shell does not split on, so
|
|
# run the output through `tr ',' '\n' | xargs -d '\n'` to hand each path to
|
|
# the tool as a distinct argument. This tolerates filenames containing
|
|
# spaces and prevents silent fallbacks: `markdownlint-cli2` would treat a
|
|
# comma-joined string as a single non-matching glob, and
|
|
# `markdown-table-formatter` would fall back to scanning every `.md` in
|
|
# the working tree when invoked with no positional args.
|
|
#
|
|
# `printf '%s\n'` is used instead of `echo` so a hypothetical leading
|
|
# `-e` or `-n` in a path is treated as data, not a bash builtin flag.
|
|
|
|
- name: lint
|
|
if: steps.changed-md.outputs.any_changed == 'true'
|
|
run: |
|
|
printf '%s\n' "$ALL_CHANGED_FILES" | tr ',' '\n' | xargs -d '\n' pnpm exec markdownlint-cli2
|
|
env:
|
|
ALL_CHANGED_FILES: ${{ steps.changed-md.outputs.all_changed_files }}
|
|
|
|
- name: fmt
|
|
if: steps.changed-md.outputs.any_changed == 'true'
|
|
run: |
|
|
printf '%s\n' "$ALL_CHANGED_FILES" | tr ',' '\n' | xargs -d '\n' pnpm exec markdown-table-formatter --check
|
|
env:
|
|
ALL_CHANGED_FILES: ${{ steps.changed-md.outputs.all_changed_files }}
|