Run all linting in Docker via Dockerfile.lint (closes #30)
All checks were successful
check / check (push) Successful in 1m2s
All checks were successful
check / check (push) Successful in 1m2s
Linting now happens in one place only: a new root Dockerfile.lint copies the repo into the digest-pinned node image already used by Dockerfile and runs eslint and prettier as build steps, so a successful build is a clean lint. script/lint is reduced to building it, which also works where the docker daemon is remote and bind mounts are impossible. No host lint path survives: the "lint" script is gone from package.json, so there is no second, unpinned way to get a lint verdict. Caching is waived for lint, because a lint build over an unchanged tree returns success in well under a second having linted nothing. LINT_EPOCH is the cache buster and it fails closed exactly as CHECK_EPOCH does: an unset ARG is the empty string, which is a perfectly stable cache key, so the guard rejects it and a bare `docker build -f Dockerfile.lint .` errors out instead of serving a green it did not earn. Both linters sit below the guard, so a fresh epoch forces them to execute while the bootstrap and dependency layers above stay cached. That makes script/lint a docker build, which nothing inside a container may call. script/check calls script/lint, so the Dockerfile image can no longer run make check: the lint stage and its COPY --from=lint ordering hack are deleted, and the remaining stage runs make test and make build under the existing CHECK_EPOCH guard. script/cibuild is now the composite gate and builds the lint image first, so a lint failure is reported before the slower suite runs. The .dockerignore exclusions are unchanged and still apply to the lint build, including the .claude/ exclusion (eslint's flat config does not ignore dot-directories, so a nested worktree in the context would be linted) and the deliberate exception that keeps .gitignore in the context for prettier. A new test asserts no per-Dockerfile ignore file shadows the root one for either image, and test/packaging/lint-docker.test.ts asserts the whole shape: the docker-only lint path, the digest pin, manifests copied before sources, the fail-closed guard with both linters below it, the absence of a lint stage or make check in Dockerfile, and the build order in script/cibuild.
This commit is contained in:
184
test/packaging/lint-docker.test.ts
Normal file
184
test/packaging/lint-docker.test.ts
Normal file
@@ -0,0 +1,184 @@
|
||||
// Linting runs in Docker, one way, everywhere: `script/lint` builds
|
||||
// `Dockerfile.lint`, which COPYs the repo into a digest-pinned image and runs
|
||||
// eslint and prettier as build steps, so a successful build IS a clean lint.
|
||||
//
|
||||
// Three things can quietly undo that, and none of them shows up as a build
|
||||
// failure, which is why they are asserted here:
|
||||
//
|
||||
// 1. Recursion. `script/check` calls `script/lint`, and `script/lint` is now a
|
||||
// `docker build`. Anything that runs `make check` inside a container is
|
||||
// therefore asking for Docker inside Docker, and CI breaks. The image built
|
||||
// from `Dockerfile` runs the suite and the compile only; lint happens once,
|
||||
// in `Dockerfile.lint`.
|
||||
// 2. Cache. A lint build over an unchanged tree returns success in well under a
|
||||
// second having linted nothing. The `LINT_EPOCH` guard is what forces the
|
||||
// linter layers to execute, and it has to fail closed: an unset build
|
||||
// argument is the empty string, which is a perfectly stable cache key, so an
|
||||
// invocation that omits it must be rejected rather than served a cached
|
||||
// green.
|
||||
// 3. A host lint path surviving alongside the container one, which would let a
|
||||
// lint result come from an unpinned local toolchain.
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { join } from "node:path";
|
||||
|
||||
const repoRoot = fileURLToPath(new URL("../../", import.meta.url));
|
||||
|
||||
const read = (name: string): string =>
|
||||
readFileSync(join(repoRoot, name), "utf-8");
|
||||
|
||||
// The executable lines of a shell script or Dockerfile: comments carry the
|
||||
// reasoning and frequently name the very commands these tests forbid, so they
|
||||
// would otherwise trigger every assertion below.
|
||||
const instructions = (name: string): string[] =>
|
||||
read(name)
|
||||
.split("\n")
|
||||
.map((line) => line.trim())
|
||||
.filter((line) => line !== "" && !line.startsWith("#"));
|
||||
|
||||
const lintScript = instructions("script/lint");
|
||||
const dockerfileLint = instructions("Dockerfile.lint");
|
||||
const dockerfile = instructions("Dockerfile");
|
||||
const cibuild = instructions("script/cibuild");
|
||||
|
||||
const has = (lines: string[], pattern: RegExp): boolean =>
|
||||
lines.some((line) => pattern.test(line));
|
||||
|
||||
describe("script/lint", () => {
|
||||
it("lints by building Dockerfile.lint", () => {
|
||||
expect(has(lintScript, /docker build .*-f Dockerfile\.lint/)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
// The whole point of the ruling: no invocation of a linter against the
|
||||
// working tree survives, so a lint verdict can only come from the pinned
|
||||
// image.
|
||||
it("runs no linter on the host", () => {
|
||||
expect(has(lintScript, /eslint|prettier/)).toBe(false);
|
||||
});
|
||||
|
||||
// Without a fresh epoch the build is served from cache in under a second,
|
||||
// having linted nothing, and still exits 0.
|
||||
it("passes a fresh LINT_EPOCH on every run", () => {
|
||||
expect(
|
||||
has(lintScript, /--build-arg LINT_EPOCH="\$\(date \+%s\)"/),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Dockerfile.lint", () => {
|
||||
// Tag references are server-mutable, so they are remote code execution.
|
||||
it("pins its base image by digest", () => {
|
||||
expect(has(dockerfileLint, /^FROM \S+@sha256:[0-9a-f]{64}/)).toBe(true);
|
||||
});
|
||||
|
||||
it("runs eslint as a build step", () => {
|
||||
expect(has(dockerfileLint, /^RUN .*eslint \./)).toBe(true);
|
||||
});
|
||||
|
||||
it("runs prettier as a build step", () => {
|
||||
expect(has(dockerfileLint, /^RUN .*prettier --check \./)).toBe(true);
|
||||
});
|
||||
|
||||
// An unset ARG is the empty string, and an empty string is a perfectly
|
||||
// stable cache key. Rejecting it is what stops a bare
|
||||
// `docker build -f Dockerfile.lint .` from reporting a green it did not
|
||||
// earn.
|
||||
it("refuses to build without LINT_EPOCH", () => {
|
||||
expect(has(dockerfileLint, /^ARG LINT_EPOCH$/)).toBe(true);
|
||||
expect(
|
||||
has(dockerfileLint, /^RUN \[ -n "\$LINT_EPOCH" \] \|\| exit 1$/),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
// The guard only forces execution of the layers below it, so both linters
|
||||
// have to sit after it. Layer order is the mechanism, not a style choice.
|
||||
it("puts both linters below the epoch guard", () => {
|
||||
const guard = dockerfileLint.findIndex((line) =>
|
||||
/^RUN \[ -n "\$LINT_EPOCH" \]/.test(line),
|
||||
);
|
||||
const linters = dockerfileLint
|
||||
.map((line, index) => ({ line, index }))
|
||||
.filter(({ line }) => /^RUN .*(eslint|prettier)/.test(line));
|
||||
|
||||
expect(linters.length).toBeGreaterThan(0);
|
||||
for (const { line, index } of linters) {
|
||||
expect(
|
||||
index,
|
||||
`${line} must run below the LINT_EPOCH guard`,
|
||||
).toBeGreaterThan(guard);
|
||||
}
|
||||
});
|
||||
|
||||
// Dependency installation is the slow layer and has nothing to do with the
|
||||
// sources, so it caches separately: manifests first, sources afterwards.
|
||||
it("copies the manifests before the sources", () => {
|
||||
const manifests = dockerfileLint.findIndex((line) =>
|
||||
/^COPY package\.json yarn\.lock/.test(line),
|
||||
);
|
||||
const sources = dockerfileLint.findIndex((line) =>
|
||||
/^COPY \. \.$/.test(line),
|
||||
);
|
||||
|
||||
expect(manifests).toBeGreaterThanOrEqual(0);
|
||||
expect(sources).toBeGreaterThan(manifests);
|
||||
});
|
||||
|
||||
// script/lint is a docker build; a lint step that shelled out to it would
|
||||
// recurse.
|
||||
it("does not call script/lint or make lint", () => {
|
||||
expect(has(dockerfileLint, /make lint|script\/lint/)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Dockerfile", () => {
|
||||
// `make check` runs script/lint, which is a docker build, so an image that
|
||||
// ran it would need a Docker daemon inside the container.
|
||||
it("does not run make check, make lint or script/lint", () => {
|
||||
expect(
|
||||
has(dockerfile, /make check|make lint|script\/(check|lint)/),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
// The replaced lint stage took a `COPY --from=lint` dependency to order
|
||||
// itself before the check stage. Dockerfile.lint is that stage now, and
|
||||
// two definitions of how to lint is one too many.
|
||||
it("has no lint stage", () => {
|
||||
expect(has(dockerfile, /AS lint\b|--from=lint\b/)).toBe(false);
|
||||
});
|
||||
|
||||
it("still runs the suite and the build under the epoch guard", () => {
|
||||
expect(has(dockerfile, /^RUN make test$/)).toBe(true);
|
||||
expect(has(dockerfile, /^RUN make build$/)).toBe(true);
|
||||
expect(
|
||||
has(dockerfile, /^RUN \[ -n "\$CHECK_EPOCH" \] \|\| exit 1$/),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("script/cibuild", () => {
|
||||
// CI has to get both verdicts. Lint goes first so the fast failure is
|
||||
// reported before the suite runs.
|
||||
it("builds the lint image before the test and build image", () => {
|
||||
const lint = cibuild.findIndex((line) => /\/lint"/.test(line));
|
||||
const check = cibuild.findIndex((line) =>
|
||||
/docker build .*CHECK_EPOCH/.test(line),
|
||||
);
|
||||
|
||||
expect(lint).toBeGreaterThanOrEqual(0);
|
||||
expect(check).toBeGreaterThan(lint);
|
||||
});
|
||||
});
|
||||
|
||||
describe("package.json", () => {
|
||||
// `yarn lint` was a second, unpinned way to get a lint verdict, from
|
||||
// whatever eslint the working tree happened to have installed.
|
||||
it("exposes no host lint script", () => {
|
||||
const pkg = JSON.parse(read("package.json")) as {
|
||||
scripts: Record<string, string>;
|
||||
};
|
||||
expect(pkg.scripts.lint).toBeUndefined();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user