diff --git a/.github/actions/prepare/action.yml b/.github/actions/prepare/action.yml new file mode 100644 index 0000000..b655b3e --- /dev/null +++ b/.github/actions/prepare/action.yml @@ -0,0 +1,94 @@ +name: 'Prepare: Node and Yarn' +description: 'Sets up Node, enables Corepack for Yarn 4, restores caches and installs dependencies.' + +# Composite action, not a reusable workflow: this runs as a *step* inside an +# existing job, so the caller keeps its own runs-on, permissions and checkout. +# +# The caller must `actions/checkout` first — this installs into whatever is +# already in the workspace. +# +# Usage: +# steps: +# - uses: actions/checkout@v4 +# - uses: iXsystems/ux-github-workflows/.github/actions/prepare@master +# with: +# cache-jest: 'true' # optional +# +# Inputs are strings, as all composite-action inputs are — compare with +# `== 'true'`, not as booleans. + +inputs: + node-version: + description: >- + Exact Node version. Pinned rather than floating on purpose: the library + and the apps that consume it should build on the same Node. + required: false + default: '24.13.1' + cache-jest: + description: "Cache .jest/cache, keyed on yarn.lock. Only useful in repos that run Jest." + required: false + default: 'false' + yarn-cache: + description: "Cache Yarn's global cache folder, keyed on yarn.lock." + required: false + default: 'false' + +runs: + using: 'composite' + steps: + # Order matters: setup-node must come before `corepack enable`. Corepack + # writes its shims into the active Node installation's bin directory, so + # enabling it first and then letting setup-node swap in a different Node + # leaves `yarn` missing. This is also why setup-node's own `cache: 'yarn'` + # is not used — it shells out to `yarn` before Corepack has run. + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: ${{ inputs.node-version }} + + - name: Enable Corepack for Yarn 4 + shell: bash + run: corepack enable + + - name: Resolve Yarn cache folder + if: inputs.yarn-cache == 'true' + id: yarn-cache-dir + shell: bash + run: | + dir="$(yarn config get cacheFolder)" + # An empty value would reach actions/cache as `path: ''` and fail there + # with a Path Validation Error that says nothing about Yarn. Fail here. + if [ -z "$dir" ]; then + echo "::error::Could not resolve the Yarn cache folder. Is this a Yarn 4 project with a packageManager field?" + exit 1 + fi + echo "dir=$dir" >> "$GITHUB_OUTPUT" + + - name: Cache Yarn packages + if: inputs.yarn-cache == 'true' + uses: actions/cache@v4 + with: + path: ${{ steps.yarn-cache-dir.outputs.dir }} + key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-yarn- + + - name: Cache Jest cache + if: inputs.cache-jest == 'true' + uses: actions/cache@v4 + with: + path: .jest/cache + key: ${{ runner.os }}-jest-${{ hashFiles('**/yarn.lock') }} + restore-keys: | + ${{ runner.os }}-jest- + + # Unconditional, and deliberately so. This step once had an `if:` guard on an + # optional install toggle; the toggle was removed and the guard was left + # behind. A composite action resolves an undeclared input to the empty + # string rather than failing, so the condition quietly became false and every + # caller got a green "Install" step that installed nothing — the jobs that + # then tried to use node_modules were the ones that failed. + # Never add an `if:` here without declaring the input it reads. + - name: Install packages + shell: bash + run: yarn install --immutable diff --git a/.github/workflows/check-member.yml b/.github/workflows/check-member.yml new file mode 100644 index 0000000..4009c36 --- /dev/null +++ b/.github/workflows/check-member.yml @@ -0,0 +1,94 @@ +name: Check Member Access (shared) + +# Reports whether the PR author has write access to the calling repo, as an +# `is_member` output. Used to route work: main.yml in truenas/webui and +# truenas-connect/ui sends team PRs to the self-hosted test runner and everyone +# else to ubuntu-latest. +# +# Usage: +# jobs: +# check-member: +# if: github.event_name == 'pull_request' +# permissions: +# contents: read +# uses: iXsystems/ux-github-workflows/.github/workflows/check-member.yml@master +# +# something: +# needs: [check-member] +# if: needs.check-member.outputs.is_member == 'true' +# +# Only meaningful on `pull_request` events — it reads +# `context.payload.pull_request`. Callers that also run on push must guard the +# job with `if: github.event_name == 'pull_request'`, and then use `always()` +# plus an explicit `!= 'true'` on the downstream job so the skip does not +# cascade. See truenas/webui's main.yml for the worked example. + +on: + workflow_call: + outputs: + is_member: + description: "'true' if the PR author has write or admin access to the calling repo." + value: ${{ jobs.check.outputs.is_member }} + +permissions: + contents: read + +jobs: + check: + # API. A reusable call reports as " / ", so consumers + # match this string in branch protection. Renaming it stops their required check + # reporting, silently, with no PR in their repo to explain it. + name: Check member access + runs-on: ubuntu-latest + outputs: + is_member: ${{ steps.check.outputs.result }} + steps: + - name: Check membership + id: check + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + result-encoding: string + script: | + // Guard first. Both the happy path and the fallback below read + // `pull_request`, so on any other event the fallback used to throw + // a second TypeError *inside* the catch — uncaught, failing the job + // rather than answering 'false'. Returning here keeps the job green + // and the `is_member` output defined for downstream `needs`. + const pullRequest = context.payload.pull_request; + if (!pullRequest) { + core.info(`No pull_request payload on a '${context.eventName}' event — reporting not-a-member.`); + return 'false'; + } + + try { + const username = pullRequest.user.login; + console.log(`Checking repository access for user: ${username}`); + + const { data: permissionLevel } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: username + }); + + console.log(`User ${username} has permission: ${permissionLevel.permission}`); + + const hasWriteAccess = ['write', 'admin'].includes(permissionLevel.permission); + console.log(`Has write access: ${hasWriteAccess}`); + + return hasWriteAccess ? 'true' : 'false'; + } catch (error) { + console.log(`Error checking permissions: ${error.message}`); + + // Fall back to the PR author association when the permission + // lookup fails (e.g. the token cannot read org membership). + // Deliberately permissive: this decides where tests run and + // whether a review happens, not whether anything merges. + const association = pullRequest.author_association; + console.log(`PR author association: ${association}`); + + const isTeamMember = ['MEMBER', 'OWNER', 'COLLABORATOR'].includes(association); + console.log(`Is team member based on association: ${isTeamMember}`); + + return isTeamMember ? 'true' : 'false'; + } diff --git a/.github/workflows/check-ticket.yml b/.github/workflows/check-ticket.yml index 4077e65..279700d 100644 --- a/.github/workflows/check-ticket.yml +++ b/.github/workflows/check-ticket.yml @@ -14,7 +14,7 @@ name: Check Ticket (shared) # # jobs: # check-ticket: -# uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@v1 +# uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@master # with: # ticket-prefixes: TNC # optional; defaults to NAS @@ -37,6 +37,9 @@ concurrency: jobs: check-ticket: + # API. A reusable call reports as " / ", so consumers + # match this string in branch protection. Renaming it stops their required check + # reporting, silently, with no PR in their repo to explain it. name: Check PR references a ticket runs-on: ubuntu-latest steps: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..94f8d40 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,69 @@ +name: CI + +# This repository had no checks of its own. Both of the workflows it publishes +# are `on: workflow_call`, and a reusable workflow never triggers on its own +# pull requests — so nothing validated this repo before a merge. +# +# That matters more here than in an ordinary repo. Consumers reference @master, +# so anything merged is live in three repos immediately, and a mistake surfaces +# as *their* CI breaking, with no pull request of their own to explain why. + +on: + pull_request: + push: + branches: + - master + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + actionlint: + name: Lint workflows + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + # Covers the workflow files: expression syntax, unknown keys, bad + # `runs-on`, shellcheck over `run:` blocks. It does NOT check the + # `action.yml` of a composite action — that gap is why the job below + # exists as well. + - name: Run actionlint + uses: docker://rhysd/actionlint:1.7.7 + with: + args: -color + + input-refs: + name: Check input references + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: '24.13.1' + + - name: Install js-yaml + run: npm install --no-save --no-package-lock js-yaml + + - name: Check every inputs.* reference is declared + run: node scripts/check-input-refs.js + + # Smoke test: this repo calls its own reusable workflow, so a change to + # check-member.yml is executed before it can be merged rather than after. + # + # Deliberately runs on push as well as pull_request. On a push there is no + # pull_request payload, which exercises the guard that reports 'false' instead + # of throwing — the path that used to fail the job outright. + self-test: + name: Self-test + permissions: + contents: read + uses: ./.github/workflows/check-member.yml diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..214f1ae --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +# CI installs js-yaml into the workspace to run scripts/check-input-refs.js +node_modules/ diff --git a/README.md b/README.md index 58fd580..0ad2309 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ on: jobs: check-ticket: - uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@v1 + uses: iXsystems/ux-github-workflows/.github/workflows/check-ticket.yml@master with: ticket-prefixes: TNC # optional; defaults to NAS ``` @@ -51,30 +51,118 @@ uppercase key, so `nas-12345` fails with a message saying so. Callers own their `on:` trigger — a reusable workflow has no say in what triggers its caller. -This one is **policy, not just plumbing.** Only `truenas/webui` requires tickets -today; `iXsystems/truenas-ui-components` deliberately treats the ticket prefix -as optional (see its `pr-title.yml`), and `truenas-connect/ui` has no PR-title -check at all. Adopt it only where the team has agreed to require tickets. +This one is **policy, not just plumbing** — it makes a ticket mandatory. All +three consumers have since agreed to that, but a fourth repo should adopt it +only once its team has. Note that `iXsystems/truenas-ui-components` requires a +ticket *and* a Conventional Commits title; the latter stays in its own local +`pr-title.yml`, since it is the only repo running semantic-release. -## Adoption status +### `check-member.yml` + +Reports whether the PR author has write access to the calling repo, as an +`is_member` output: -| Repo | `check-ticket.yml` | +```yaml +jobs: + check-member: + if: github.event_name == 'pull_request' + permissions: + contents: read + uses: iXsystems/ux-github-workflows/.github/workflows/check-member.yml@master + + test-ux-team: + needs: [check-member] + if: needs.check-member.outputs.is_member == 'true' + runs-on: self-hosted + # ... +``` + +| Output | Notes | |---|---| -| `truenas/webui` | migrating (first adopter) | -| `iXsystems/truenas-ui-components` | n/a — tickets optional there | -| `truenas-connect/ui` | n/a — no PR-title check | +| `is_member` | `'true'` / `'false'` — a string, not a boolean. Compare with `== 'true'` | -## Releasing +`main.yml` in `truenas/webui` and `truenas-connect/ui` calls it to route tests +to the self-hosted runner. Those were three separate copies of the same script +before this existed — two workflow files plus one inlined directly in +`truenas-connect/ui`'s `main.yaml`. -Callers pin `@v1`, so a change reaches them only when the tag moves: +Only meaningful on `pull_request` events: it reads +`context.payload.pull_request`, and reports `'false'` on any event that has no +PR payload rather than failing. Guarding the job with +`if: github.event_name == 'pull_request'` is still worth doing to skip a +pointless runner — but then the downstream job needs `always()` (or +`!cancelled()`) plus an explicit `!= 'true'`, so the skip does not cascade into +it. See `truenas/webui`'s `main.yml` for the worked example. -```bash -git tag -f v1 && git push -f origin v1 +If the permission lookup fails it falls back to `author_association`, which is +deliberately permissive. It decides where tests run; it must not be +load-bearing for anything that gates a merge. + +## Actions + +### `.github/actions/prepare` + +A **composite action**, not a reusable workflow: it runs as a step inside an +existing job, so the caller keeps its own `runs-on`, `permissions` and checkout. +Reusable workflows cannot do that — they bring their own job. + +```yaml +steps: + - uses: actions/checkout@v4 # required first; this installs into the workspace + - uses: iXsystems/ux-github-workflows/.github/actions/prepare@master + with: + cache-jest: 'true' # optional ``` -Land the change on `master`, verify it against the first adopter's next PR, then -move the tag. For a breaking input change, cut `v2` and migrate callers one at a -time instead. +| Input | Default | Notes | +|---|---|---| +| `node-version` | `24.13.1` | Pinned, not floating | +| `cache-jest` | `'false'` | Caches `.jest/cache`; only useful where Jest runs | +| `yarn-cache` | `'false'` | Caches Yarn's global cache folder | + +Inputs are strings — every composite-action input is. Compare with `== 'true'`. + +**Step order is load-bearing.** `actions/setup-node` runs *before* +`corepack enable`, because Corepack writes its shims into the active Node +installation's bin directory: enable it first and then let setup-node swap in a +different Node, and `yarn` goes missing. That is also why setup-node's own +`cache: 'yarn'` is not used — it shells out to `yarn` before Corepack has run, +and would either fail or silently cache Yarn 1's directory for a Yarn 4 repo. +The `yarn-cache` input resolves the folder with `yarn config get cacheFolder` +after Corepack instead. + +This replaced identical local copies in `truenas/webui` and `truenas-connect/ui` +and six inline repetitions in `iXsystems/truenas-ui-components`'s `ci-cd.yml`, +which had drifted to a floating `'24'` against the others' pinned `24.13.1`. + +## Adoption status + +| Repo | `check-ticket` | `check-member` | `prepare` | +|---|---|---|---| +| `truenas/webui` | adopted | migrating (`main.yml`) | migrating | +| `iXsystems/truenas-ui-components` | adopted | n/a — no self-hosted runner | migrating | +| `truenas-connect/ui` | adopted | migrating (`main.yaml`) | migrating | + +**Automatic PR review is deliberately not here yet.** Each repo keeps its own +`claude.yml` while that work is in flight; revisit sharing it once those changes +have settled. + +## Releasing + +Callers reference `@master`, so **anything landing on `master` is live in every +consumer immediately** — there is no per-repo review gate between a change here +and three repos' CI running it. + +That puts the whole burden on the PR into this repo: + +- Treat a change to a job `name:` as breaking. Consumers match + `" / "` in branch protection, so a rename silently + stops a required check reporting, with no PR in their repo to explain it. +- Same for removing or renaming an input, or tightening a default. +- Verify against one consumer's next real PR before assuming it is fine + everywhere; the consumers differ in trigger, secret names and permissions. -Tags are the release surface here, not branches — a caller pinned to `@master` -would pick up unreviewed changes on every push to every consumer at once. +If that becomes too sharp an edge, the alternative is tagging: cut `v1`, move +callers to `@v1`, and release with `git tag -f v1 && git push -f origin v1`. +That was the original intent, but with only three consumers and one team it was +judged more ceremony than it buys. diff --git a/scripts/check-input-refs.js b/scripts/check-input-refs.js new file mode 100644 index 0000000..933f302 --- /dev/null +++ b/scripts/check-input-refs.js @@ -0,0 +1,107 @@ +#!/usr/bin/env node +// +// Fails if any `inputs.` reference resolves to an input that is not +// declared in the same file. +// +// This exists because of a real failure. The `prepare` action once had an +// optional install toggle; the input was removed and `if: inputs.install == +// 'true'` was left behind on the step. GitHub resolves an undeclared input to +// the empty string rather than erroring, so the condition quietly became false, +// the install step was skipped, and the job reported a green "Install" that had +// installed nothing. It surfaced two repos away, as a webui build failing on a +// missing node_modules. +// +// actionlint does not cover this case: it checks workflow files, not the +// `action.yml` of a composite action — which is exactly where the bug was. + +const fs = require('fs'); +const path = require('path'); +const yaml = require('js-yaml'); + +function targets() { + const found = []; + const workflows = '.github/workflows'; + if (fs.existsSync(workflows)) { + for (const f of fs.readdirSync(workflows)) { + if (/\.ya?ml$/.test(f)) found.push(path.join(workflows, f)); + } + } + const actions = '.github/actions'; + if (fs.existsSync(actions)) { + for (const dir of fs.readdirSync(actions)) { + for (const name of ['action.yml', 'action.yaml']) { + const p = path.join(actions, dir, name); + if (fs.existsSync(p)) found.push(p); + } + } + } + return found.sort(); +} + +function declaredInputs(doc) { + // YAML 1.1 parses a bare `on:` key as the boolean true, so a workflow's + // trigger block lands on doc[true] rather than doc.on. + const on = doc[true] || doc.on || {}; + const sources = [ + doc.inputs, // composite action + on.workflow_call && on.workflow_call.inputs, + on.workflow_dispatch && on.workflow_dispatch.inputs, + ]; + return new Set(sources.filter(Boolean).flatMap((s) => Object.keys(s))); +} + +function referencedInputs(raw) { + // Drop whole-line comments first, so prose describing a removed input is not + // mistaken for a live reference. Only full-line comments are stripped — a `#` + // mid-line may be inside a string. + const code = raw + .split('\n') + .filter((line) => !/^\s*#/.test(line)) + .join('\n'); + return new Set([...code.matchAll(/inputs\.([A-Za-z0-9_-]+)/g)].map((m) => m[1])); +} + +let failures = 0; + +for (const file of targets()) { + const raw = fs.readFileSync(file, 'utf8'); + + let doc; + try { + doc = yaml.load(raw); + } catch (error) { + console.log(`FAIL ${file}\n unparseable YAML: ${error.message}`); + failures++; + continue; + } + + const declared = declaredInputs(doc); + const referenced = referencedInputs(raw); + const undeclared = [...referenced].filter((name) => !declared.has(name)); + const unused = [...declared].filter((name) => !referenced.has(name)); + + if (undeclared.length) { + failures++; + console.log(`FAIL ${file}`); + for (const name of undeclared) { + console.log(` references \`inputs.${name}\`, which is not declared.`); + console.log(' GitHub resolves this to an empty string — it will not error at runtime,'); + console.log(' it will silently evaluate as falsy. Declare the input or drop the reference.'); + } + } else { + console.log(`ok ${file}`); + } + + // Not a failure: an input can be a deliberate escape hatch no caller uses yet. + // Still worth surfacing, since an unused input is also how the above starts. + for (const name of unused) { + console.log(` note: \`${name}\` is declared but never referenced in this file.`); + } +} + +if (failures) { + console.log(`\n${failures} file(s) reference undeclared inputs.`); + process.exit(1); +} + +console.log('\nAll input references resolve.');