From 327654e57bc7264e0d81b2414776d681ec30fe02 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 6 Oct 2026 22:09:25 +0200 Subject: [PATCH] Fail a touched manifest's module check only for what the change brings (hq ADR 0237) de-spiegel's and link2pay's manifests already fail the module check on main; without comparing against the base branch every pull request touching them would fail the gate for a fault none of them made. The gate's own rule: what was already so is said. --- cmd/mesh-builder/main.go | 2 +- internal/builder/check.go | 91 ++++++++++++++++++++++++++++------ internal/builder/check_test.go | 31 ++++++++++++ 3 files changed, 109 insertions(+), 15 deletions(-) diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index dddf2f8..e6a826a 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -334,7 +334,7 @@ func checkSpecOf(request link.BuildRequest) builder.CheckSpec { c := request.Check spec := builder.CheckSpec{ID: request.ID, Repository: request.Repository, Ref: request.Ref, Owner: c.Owner, Repo: c.Repo, Number: c.Number, Paths: c.Paths, Beside: map[string]builder.Beside{}, - Modules: c.Modules, New: c.New, Manifests: c.Manifests, Judge: c.Judge, + Modules: c.Modules, New: c.New, Manifests: c.Manifests, Judge: c.Judge, Base: c.Base, Toolchain: builder.ToolchainOf(request.Held), Toolchains: builder.ToolchainsOf(request.Held)} for dir, b := range c.Beside { spec.Beside[dir] = builder.Beside{Repository: b.Repository, Ref: b.Ref} diff --git a/internal/builder/check.go b/internal/builder/check.go index 8d4beef..5857363 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -72,6 +72,8 @@ type CheckSpec struct { Modules []string New []string Manifests []string + // Base is the branch the pull request merges into: what the gate compares the change against. + Base string // Judge is who judges the gate: "" the controller the mesh runs, "self" the change's own, "validator" // the running one with the change's validator. Judge string @@ -390,7 +392,10 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFile string, env []string, inToolchain func(string, []string, ...string) []string, running func([]string) error, out *tail, bus, workspace, name string, say func(step, format string, args ...any)) (string, string) { - // 1. The manifests the change touches, as the judge reads them: a manifest it cannot read fails here. + // 1. The manifests the change touches, as the judge reads them: a manifest it cannot read, or one with a + // problem the change brings, fails here. **What was already so on the base branch is said and fails + // nothing** — the gate's own rule: a module whose manifest the running controller already finds fault + // with would otherwise fail every pull request that touches it, for a fault none of them made. var manifests []string for _, m := range spec.Manifests { if _, err := os.Stat(filepath.Join(tree, m)); err == nil { @@ -398,17 +403,44 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil } } if len(manifests) > 0 { - var own tail - cmd := exec.CommandContext(ctx, "docker", LabelledArgs("docker", - inToolchain(tree, env, append([]string{gate, "module", "check"}, manifests...)...), spec.ID)...) - inItsOwnGroup(cmd) - w := io.MultiWriter(out, &own) - cmd.Stdout, cmd.Stderr = w, w - if err := cmd.Run(); err != nil { + checked := func(dir string) (string, error) { + var own tail + cmd := exec.CommandContext(ctx, "docker", LabelledArgs("docker", + inToolchain(dir, env, append([]string{gate, "module", "check"}, manifests...)...), spec.ID)...) + inItsOwnGroup(cmd) + w := io.MultiWriter(out, &own) + cmd.Stdout, cmd.Stderr = w, w + err := cmd.Run() + return own.String(), err + } + said, err := checked(tree) + if err != nil { if ctx.Err() != nil { return "error", "the check was ended during the module check" } - return "fail", "a manifest the change touches fails the module check: " + firstProblem(own.String()) + // The same manifests as the base branch has them, beside the change. + was := "" + if spec.Base != "" { + base := filepath.Join(root, "base-manifests") + for _, m := range manifests { + body, err := gitShow(ctx, tree, "origin/"+spec.Base, m) + if err != nil { + continue // new on this branch: nothing was so before it + } + if err := os.MkdirAll(filepath.Dir(filepath.Join(base, m)), 0o755); err == nil { + _ = os.WriteFile(filepath.Join(base, m), body, 0o644) + } + } + fmt.Fprintf(out, "--- the same manifests on %s\n", spec.Base) + if _, err := os.Stat(base); err == nil { + was, _ = checked(base) + } + } + brought := newProblems(said, was) + if len(brought) > 0 { + return "fail", "a manifest the change touches fails the module check: " + brought[0] + } + fmt.Fprintf(out, "the module check's problems were all so on %s already: said, not the change's\n", spec.Base) } } @@ -551,15 +583,46 @@ func replaceGoFiles(from, into string) error { return nil } -// firstProblem is the first line of a module check's output that names a problem. -func firstProblem(s string) string { +// problemLines are the lines of a module check's output that name a problem: not a manifest found ok, not +// the count, not the closing note. +func problemLines(s string) []string { + var out []string for _, line := range strings.Split(s, "\n") { line = strings.TrimSpace(line) - if line != "" && !strings.HasSuffix(line, "tool(s)") && !strings.Contains(line, ": ok") { - return line + switch { + case line == "", strings.Contains(line, ": ok"), strings.Contains(line, "problem(s) in"), + strings.Contains(line, "manifest(s) checked"): + continue + } + out = append(out, line) + } + return out +} + +// newProblems are the problems a module check said of the change that it did not say of the base. +func newProblems(change, base string) []string { + was := map[string]bool{} + for _, p := range problemLines(base) { + was[p] = true + } + var out []string + for _, p := range problemLines(change) { + if !was[p] { + out = append(out, p) } } - return lastLine(s) + if len(out) == 0 && len(problemLines(change)) == 0 { + // It failed and named nothing: not readable as already so. + out = append(out, lastLine(change)) + } + return out +} + +// gitShow is a file as a ref has it. +func gitShow(ctx context.Context, dir, ref, file string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", "show", ref+":"+filepath.ToSlash(file)) + cmd.Dir = dir + return cmd.Output() } func prefixed(prefix string, items []string) []string { diff --git a/internal/builder/check_test.go b/internal/builder/check_test.go index d96feaf..b7eb233 100644 --- a/internal/builder/check_test.go +++ b/internal/builder/check_test.go @@ -292,6 +292,37 @@ func TestTheGateRunsWhenTheGraphIsTouchedBesideTheRepositorysOwnCheck(t *testing t.Fatalf("a manifest the judge refuses answered %+v / %+v\n%s", v.Gate, v.Repo, v.Report) } + // A fault the base branch already had is said and fails nothing; one the change brings fails. + repo, _ := aCheckedRepository(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea","broken":true}`}) + git := func(args ...string) string { + cmd := exec.Command("git", args...) + cmd.Dir = repo + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.org", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.org") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) + } + git("checkout", "--quiet", "-b", "change") + if err := os.WriteFile(filepath.Join(repo, "modules/gitea/index.ts"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + git("add", "-A") + git("commit", "--quiet", "-m", "y") + id := fmt.Sprintf("check-base-%d", time.Now().UnixNano()) + v, err := Check(t.Context(), Command, CheckSpec{ID: id, Repository: repo, Ref: git("rev-parse", "HEAD"), Owner: "novox", + Repo: "mesh-catalog", Base: "main", Paths: []string{"modules/gitea/index.ts"}, Beside: beside, + Modules: []string{"gitea"}, Manifests: []string{"modules/gitea/module.json"}, Toolchain: goToolchain}, + t.TempDir(), registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate.Verdict != "warning" || !strings.Contains(v.Report, "already") { + t.Fatalf("a fault the base already had failed the change: %+v\n%s", v.Gate, v.Report) + } + // A change to the controller judges itself: one that does not build fails its own gate. v = check(map[string]string{"go.mod": "module x\n\ngo 1.22\n", "cmd/mesh-controller/main.go": "package main\nfunc main() { nope }\n", "modules/gitea/module.json": "{}"}, "self")