From 4357b3688f586fd93c0c53c587f1d6f2dfd83fb5 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 11:07:34 +0200 Subject: [PATCH] Tell mesh-config what a head declares at every head, so a head that reverts a key drops its held value (issue 1826) --- cmd/mesh-controller/checks.go | 17 ++++++++---- cmd/mesh-controller/pull_declares.go | 34 ++++++++++++++++------- cmd/mesh-controller/pull_declares_test.go | 31 +++++++++++++++++++-- 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index c715e5d1..a408e842 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -191,6 +191,16 @@ func (f following) pullUpdated(ctx context.Context, p link.PullUpdated, w io.Wri scope.Modules, scope.Dependents, scope.New, scope.Manifests = nil, nil, nil, nil composedGate(&direct) } + // **Mesh-config is told what the head declares at every announced head of a pull request in a repository that holds + // modules** (novox/hq ADR 0502 clauses 2 and 6, issue 1826), also when the head touches no module: a head that reverts + // the change must drop the value held for it. Asked of nothing else here. + this := pullAtOf(p) + if this.Truncated { + this.Paths, this.Truncated = truncatedPaths(entries, p.Owner, p.Repo, scope.Modules), false + } + if scope.gated() || holdsModules(entries, p.Owner, p.Repo) { + tellPullDeclares(ctx, w, []pullAt{this}) + } switch { case !scope.gated() && !scope.Mesh: fmt.Fprintf(w, "%s/%s#%d (%.8s): %s, and the repository is not the mesh's: said, nothing run\n", @@ -209,12 +219,9 @@ func (f following) pullUpdated(ctx context.Context, p link.PullUpdated, w io.Wri } request.Check.Plan = &plan request.Check.Composed = composed - // **A value held for a key this pull request introduces is told and carried** (novox/hq ADR 0502, issue 1826). - this := pullAtOf(p) - if this.Truncated { - this.Paths, this.Truncated = truncatedPaths(entries, p.Owner, p.Repo, scope.Modules), false + if !composed { + carryHeld(ctx, w, &request, []pullAt{this}) } - withHeldConfig(ctx, w, &request, []pullAt{this}, !composed) // **The group's other open pull requests are read beside it** (novox/hq ADR 0382 ยง1). siblingsFrom(&request, p, entries, lines, groupsErr) seat := buildSeatHeld(ctx) diff --git a/cmd/mesh-controller/pull_declares.go b/cmd/mesh-controller/pull_declares.go index fb882a45..b639a6b3 100644 --- a/cmd/mesh-controller/pull_declares.go +++ b/cmd/mesh-controller/pull_declares.go @@ -68,12 +68,11 @@ func pullKey(owner, repo string, number int) string { func (p pullAt) key() string { return pullKey(p.Owner, p.Repo, p.Number) } // pullDeclarations are the config declarations a pull request's head adds or changes over what it merges into, by -// module, in the shape of the declared-config mirror. The bool says the pull request changes a module.json that exists -// at its head: mesh-config is told then, even of no declaration, so what it kept for an earlier head is replaced. A -// pull request whose paths are truncated is read through the manifests of the modules it moves (the caller's paths). +// module, in the shape of the declared-config mirror. The bool says the head could be read: false for a pull request whose +// paths are cut (the caller cuts them to the manifests of the modules it moves) or whose base is unknown, which is then +// told nothing, never an empty set. func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfigMirrored, bool, error) { var out []inventory.DeclaredConfigMirrored - touched := false if p.Truncated || p.Old == "" { return nil, false, nil } @@ -88,7 +87,6 @@ func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfig if !exists || now.Module == "" { continue } - touched = true was, _, err := manifestCached(ctx, p.Owner, p.Repo, file, p.Old) if err != nil { return nil, false, err @@ -111,19 +109,21 @@ func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfig out = append(out, inventory.DeclaredConfigMirrored{Module: now.Module, Settings: changed}) } } - return out, touched, nil + return out, true, nil } -// tellPullDeclares tells mesh-config, for each pull request that changes a module.json, what its head declares. A +// tellPullDeclares tells mesh-config, for each pull request at every announced head, what the head declares, an empty set +// when it declares nothing new: mesh-config re-judges the values held at every head, so a head that reverts the change +// drops them (ADR 0502 clause 6). A // failure is said to w and the check goes on: the held values are then none (ADR 0502 clause 2). func tellPullDeclares(ctx context.Context, w io.Writer, pulls []pullAt) { for _, p := range pulls { - mods, touched, err := pullDeclarations(ctx, p) + mods, readable, err := pullDeclarations(ctx, p) if err != nil { fmt.Fprintf(w, "%s: what its head declares cannot be read, so mesh-config is told nothing: %v\n", p.key(), err) continue } - if !touched { + if !readable { continue } declares := map[string]map[string]inventory.DeclaredMirrored{} @@ -191,7 +191,21 @@ func heldFor(ctx context.Context, w io.Writer, pulls []pullAt) []link.HeldConfig // declarations are told. func withHeldConfig(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt, carry bool) { tellPullDeclares(ctx, w, pulls) - if carry && request.Check != nil { + if carry { + carryHeld(ctx, w, request, pulls) + } +} + +// carryHeld puts the approved held values of these pull requests on the check request. +func carryHeld(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt) { + if request.Check != nil { request.Check.HeldConfig = heldFor(ctx, w, pulls) } } + +// holdsModules is whether a module of the mesh is built from this repository. +func holdsModules(entries []inventory.Entry, owner, repo string) bool { + return slices.ContainsFunc(entries, func(e inventory.Entry) bool { + return sameRepository(e.Source.Repository, link.SourceMoved{Owner: owner, Repo: repo}) + }) +} diff --git a/cmd/mesh-controller/pull_declares_test.go b/cmd/mesh-controller/pull_declares_test.go index 5e4b4d9d..c325e79a 100644 --- a/cmd/mesh-controller/pull_declares_test.go +++ b/cmd/mesh-controller/pull_declares_test.go @@ -98,8 +98,9 @@ func TestAnUnchangedKeyIsNotToldAndAChangedOneIs(t *testing.T) { } } -// 1826/AC1: a pull request that changes no module.json, or removes it, tells mesh-config nothing. -func TestAPullRequestThatDeclaresNothingSendsNothing(t *testing.T) { +// 1826/AC1: a head that changes no module.json, or removes it, is still told, with declares {}: mesh-config re-judges at every +// head, so a head that reverts the change drops the value held for it (review round 1, finding 1). +func TestAHeadThatDeclaresNothingIsToldAsEmpty(t *testing.T) { fakeManifests(t, map[string]*catalogue.Manifest{baseSHA: manifest("n8n", nil), headSHA: manifest("n8n", nil)}) calls := fakeConfig(t, nil) p := thePull() @@ -108,9 +109,25 @@ func TestAPullRequestThatDeclaresNothingSendsNothing(t *testing.T) { p = thePull() p.Removed = p.Paths tellPullDeclares(context.Background(), io.Discard, []pullAt{p}) - if len(*calls) != 0 { + if len(*calls) != 2 { t.Fatalf("mesh-config was told %v", *calls) } + for _, c := range *calls { + if d, _ := json.Marshal(c["declares"]); string(d) != "{}" || c["head"] != headSHA { + t.Fatalf("told %v", c) + } + } +} + +// 1826/AC1: a head whose paths are cut, or whose base is unknown, cannot be read: nothing is told, never an empty set. +func TestAHeadThatCannotBeReadIsNotToldAsEmpty(t *testing.T) { + calls := fakeConfig(t, nil) + p := thePull() + p.Truncated = true + tellPullDeclares(context.Background(), io.Discard, []pullAt{p}) + if len(*calls) != 0 { + t.Fatalf("told %v", *calls) + } } // 1826/AC1: a module.json changed without a declaration changing is told as an empty set, so mesh-config re-judges what it holds. @@ -182,3 +199,11 @@ func TestTheCheckRequestCarriesTheHeldValues(t *testing.T) { t.Fatalf("a composed member carries %+v", request.Check.HeldConfig) } } + +// 1826/AC1: a repository that builds a module of the mesh holds modules; another does not. +func TestARepositoryThatBuildsAModuleHoldsModules(t *testing.T) { + entries := []inventory.Entry{{Manifest: catalogue.Manifest{Module: "a"}, Source: inventory.Source{Repository: "novox/mesh-catalog", Path: "modules/a"}}} + if !holdsModules(entries, "novox", "mesh-catalog") || holdsModules(entries, "novox", "hq") { + t.Fatal("wrong") + } +}