ci: guard the exported API against accidental breaking changes - #1297
Conversation
The build and the tests do not catch every break. Adding a trailing variadic parameter to an exported function changes that function's type: every ordinary call site still compiles, so nothing goes red, while any caller that held the function as a value stops compiling. That reached a release, and a test written specifically to pin backward compatibility did not catch it either, because it was an ordinary call expression. The check runs apidiff over every exported package and fails on incompatible changes. Adding to the API is always allowed. Two details that matter. It compares against the merge base rather than the target branch tip, because a branch cut before a recent change to the target would otherwise be reported as removing whatever the target gained in the meantime; the first draft did exactly that and produced two convincing false positives. And internal packages are excluded, since they are not public API. Verified end to end rather than assumed: run against the commit that introduced the variadics it reports both constructors and exits 1, and against the commit that restores them it exits 0 with no findings.
zizmor's template-injection audit flagged the run step, correctly. A
${{ }} expansion inside run: is substituted into the shell text before
the shell sees it, so whatever the value holds becomes code rather than
an argument. Passing it as an environment variable and quoting it in the
script keeps it data.
Caught by CI on the pull request that added the workflow, which is a
reasonable advertisement for the check that job performs.
The check shipped with no way to land an intended break. The failure message said a maintainer could override it, but the only mechanism was disabling a required check, which is how checks get removed rather than overridden. A pull request labelled breaking-change is still compared and still prints what changed, so the break is on the record instead of waved through invisibly. It just stops failing.
The check took eleven minutes warm and over fifteen cold. It invoked apidiff once per exported package, and every invocation reloads and type-checks the entire dependency graph. apidiff -m reads a whole module in one pass and reports the same findings in seven seconds. Module mode also reaches plugin/agentanalytics, which go list ./... never saw. That module is public API and was going unchecked. And it skips internal/ on its own, so the grep filtering is gone with it. Separately, a module that cannot be read now fails. Every apidiff call carried a trailing || true, so a base revision that failed to build wrote no snapshots, every package was then skipped for want of one, and the job went green having compared nothing at all. apidiff exits non-zero only when it could not do its job, so that case is easy to separate from a run that found a break, and the breaking-change label does not cover it: a comparison that did not happen is not a break anyone decided to make. Verified against the same cases as before. A trailing variadic added to runner.New and to NewBatchProcessor is reported and exits 1, a clean tree exits 0, and a module that does not compile fails even with the label set.
|
Pushed 3c1d2e2, which changes how the comparison is driven. The description above still describes the per-package version, so, in short: One It now covers A module that cannot be read now fails the check. Every Re-verified the same cases: clean tree exits 0; a trailing variadic on Backport to Generated with CloudCode, session |
#1304) Backport of #1297 to the 1.x maintenance branch, where the guarantee it enforces matters more than it does on main: v1 is a maintenance line, so every break in it is by definition unintended. The build and the tests do not catch every break. Adding a trailing variadic parameter to an exported function changes that function's type: every ordinary call site still compiles, so nothing goes red, while any caller that held the function as a value stops compiling. That reached a release, and a test written specifically to pin backward compatibility did not catch it either, because a plain call expression is exactly the form a trailing variadic can never break. The check runs apidiff over every module and fails on incompatible changes. Adding to the API is always allowed. A deliberate break can be landed by labelling the pull request breaking-change, which still compares and still prints what changed, so the break stays on the record. The workflow already listed v1 among its trigger branches, but a pull_request workflow runs from the head of the branch under test, so it could never fire on a v1 pull request until it existed here. Both files are byte-identical to the ones on main.
2 participants
What
Adds an
apidiffcheck that fails a pull request on incompatible changes to the module's exported API. Adding to the API is always allowed.Why
The build and the tests do not catch every break. Adding a trailing variadic parameter to an exported function changes that function's type: every ordinary call site still compiles, so nothing goes red, while any caller that held the function as a value stops compiling.
That is not hypothetical. It happened to
NewRuntimeAPIController,NewPubSubControllerandNewEventarcController, and reached a release. A test written specifically to pin backward compatibility did not catch it either, because a plain call expression is exactly the form a trailing variadic can never break.Two details worth reviewing
origin/mainand produced two convincing false positives on a branch that had been cut before fix(session): give Event a consistent JSON encoding #1252 landed, reporting it as having removed the methods that PR added. A check that cries wolf gets ignored, so this seemed worth getting right.internal/packages are excluded, since they are not public API.Testing Plan
Verified end to end against real commits rather than a synthetic case:
apidiffversion resolves and installs from a clean module.bash -n.Local run on the breaking commit:
Notes
This is independent of the context-compaction stack (#1231 to #1236), though that is where the motivating break was found and fixed.