diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index 2f5505e9..c7065e80 100644 --- a/cmd/mesh-controller/batches.go +++ b/cmd/mesh-controller/batches.go @@ -939,11 +939,16 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ commits, named := carriedOf(merges) var moved []string var members []batchMember + unchanged := map[string]string{} for _, m := range combinedMerges(merges) { // **News, whatever was seen since** (review of this change): each merge was judged when it was heard, and // a cut, a fold or a failed walk since marks its modules seen — so a merge walked alone after a failed // walk, or folded, would read as history and move nothing. When it was made is not asked again here. m.MergedAt = "" + // **Unchanged, said** (novox/hq issue 611): read before the merge is acted on, from the same records. + for n, why := range unchangedOfMerge(m, entries, read) { + unchanged[n] = why + } names, err := movesOfMerge(ctx, inv, m, entries, read) if err != nil { return err @@ -995,12 +1000,16 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ plan.Delivery.Merges = named plan.Delivery.Alone = batch.Delivery != nil && batch.Delivery.Alone plan.Delivery.Withheld = withheld + plan.Delivery.Unchanged = unchangedLeft(unchanged, plan.Modules) // **Its moments and its class** (novox/hq ADR 0282 decision 6): measured, never acted on. plan.Times = walkTimesAtCut(*batch, plan, entries, now) if len(moved) == 0 { plan.State = inventory.PlanDone plan.Tiers = [][]string{} plan.Note = "moved nothing the mesh holds: nothing to walk" + if len(plan.Delivery.Unchanged) > 0 { + plan.Note = "moved nothing the mesh holds: nothing to walk; " + unchangedSaid(plan.Delivery.Unchanged) + } if err := inv.SavePlan(ctx, &plan); err != nil { return err } @@ -1045,6 +1054,31 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ return nil } +// unchangedSaid names the modules a walk says matched what runs, for its note. +func unchangedSaid(unchanged map[string]string) string { + names := make([]string, 0, len(unchanged)) + for n := range unchanged { + names = append(names, n) + } + sort.Strings(names) + return "unchanged, the build the mesh holds is theirs: " + strings.Join(names, ", ") +} + +// unchangedLeft is what a walk says matched what runs: every module a merge left unchanged that the walk does not +// build anyway, as another merge's module or a dependent (novox/hq issue 611). Nil when none is. +func unchangedLeft(unchanged map[string]string, builds map[string]*inventory.PlanModule) map[string]string { + out := map[string]string{} + for n, why := range unchanged { + if _, built := builds[n]; !built { + out[n] = why + } + } + if len(out) == 0 { + return nil + } + return out +} + // withhold takes from a plan every module named that its merges do not move, and the tiers left empty: the // modules taken. func withhold(plan *inventory.Plan, names, moved []string) []string { diff --git a/cmd/mesh-controller/issue611_test.go b/cmd/mesh-controller/issue611_test.go new file mode 100644 index 00000000..2b2a8e7b --- /dev/null +++ b/cmd/mesh-controller/issue611_test.go @@ -0,0 +1,145 @@ +package main + +import ( + "reflect" + "sort" + "strings" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/inventory" + "github.com/novox/mesh-controller/internal/link" +) + +// novox/hq issue 611: mesh-controller #293 changed one test file. Its delivery plan, made while another walk built +// build-agent and route-proxy (issue 606), read them whole and named them; at the cut their recorded build sources +// held none of its files, so its walk moved nothing and its record named no module. mesh-delivery could not tell +// "matched what runs" from "never built" and stopped the delivery, asking the operator about a change already on +// every machine. The walk now says, per module, that its build matched what runs (delivery.unchanged, ADR 0267 §5). + +// The shape of #293: a test file of the controller's repository. The controller, built from the root, and the two +// modules packaging the repository each recorded a build source holding none of it: each is said unchanged, with +// why; a module of another repository is not named. +func TestAMergeSaysTheModulesWhoseBuildSourceItLeftUnchanged(t *testing.T) { + const catalogueRepo = "http://forge.internal:20000/novox/mesh-catalog.git" + const controllerRepo = "http://forge.internal:20000/novox/mesh-controller.git" + entries := []inventory.Entry{ + fromRepo("mesh-controller", controllerRepo, ""), + fromRepo("build-agent", catalogueRepo, "modules/build-agent"), + fromRepo("route-proxy", catalogueRepo, "modules/route-proxy"), + fromRepo("gitea", catalogueRepo, "modules/gitea"), + } + read := map[string][]inventory.ReadRepository{ + "mesh-controller": {{Own: true, Paths: []string{"go.mod", "module.json", "cmd/mesh-controller/", "internal/builder/"}}}, + "build-agent": { + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"go.mod", "cmd/mesh-builder/", "internal/builder/"}}, + {Own: true, Paths: []string{"modules/build-agent/Dockerfile", "modules/build-agent/module.json"}}, + }, + "route-proxy": { + {Repository: "novox/mesh-controller", Ref: "main", Paths: []string{"go.mod", "examples/route-proxy/"}}, + {Own: true, Paths: []string{"modules/route-proxy/Dockerfile", "modules/route-proxy/module.json"}}, + }, + } + m := link.SourceMoved{Owner: "novox", Repo: "mesh-controller", Base: "main", Commit: "8f1aec0", + Paths: []string{"internal/builder/replay363_test.go"}} + if moved := reachOfMerge(m, entries, read, sharedRepositoryEdges).Moved(); len(moved) != 0 { + t.Fatalf("the fixture moves %v: a test file is in no build source", moved) + } + got := unchangedOfMerge(m, entries, read) + var names []string + for n, why := range got { + names = append(names, n) + if !strings.Contains(why, "holds none of") || !strings.Contains(why, "internal/builder/replay363_test.go") || + !strings.Contains(why, "the build the mesh holds is its build") { + t.Errorf("%s is said unchanged as %q", n, why) + } + } + if want := []string{"build-agent", "mesh-controller", "route-proxy"}; !reflect.DeepEqual(sortedOf(names), want) { + t.Fatalf("said unchanged: %v; want %v", sortedOf(names), want) + } + + // A module whose build source holds a changed file is moved, never said unchanged. + touched := m + touched.Paths = []string{"internal/builder/builder.go"} + if got := unchangedOfMerge(touched, entries, read); got["mesh-controller"] != "" || got["build-agent"] != "" { + t.Errorf("a module the merge moves is said unchanged: %v", got) + } + if got := unchangedOfMerge(touched, entries, read); got["route-proxy"] == "" { + t.Errorf("the route proxy, whose build source holds none of it, is not said unchanged: %v", got) + } + // With no build source recorded, the module is read whole and moved: nothing says it unchanged. + unsaid := map[string][]inventory.ReadRepository{"build-agent": {{Repository: "novox/mesh-controller", Ref: "main"}}} + if got := unchangedOfMerge(m, entries, unsaid); len(got) != 0 { + t.Errorf("modules with no recorded build source are said unchanged: %v", got) + } + // Files not all said: every module is moved, none said unchanged. + truncated := m + truncated.PathsTruncated = true + if got := unchangedOfMerge(truncated, entries, read); len(got) != 0 { + t.Errorf("a merge whose files were not all said says %v unchanged", got) + } +} + +// A module the walk builds anyway — another merge's, or a dependent — is not said unchanged: the modules map keeps +// meaning what the walk builds, and the two never name one module. +func TestAWalkSaysUnchangedOnlyWhatItDoesNotBuild(t *testing.T) { + unchanged := map[string]string{"build-agent": "why a", "route-proxy": "why b"} + got := unchangedLeft(unchanged, map[string]*inventory.PlanModule{"build-agent": {}}) + if !reflect.DeepEqual(got, map[string]string{"route-proxy": "why b"}) { + t.Fatalf("left %v", got) + } + if got := unchangedLeft(map[string]string{"x": "y"}, map[string]*inventory.PlanModule{"x": {}}); got != nil { + t.Fatalf("left %v; want none", got) + } +} + +// Against the store, the path a real merge takes: a merge whose module recorded a build source that holds none +// of its files is cut into a walk done with nothing to walk, whose record names the module unchanged and why. +func TestAWalkThatMovedNothingSaysWhichModulesMatchedWhatRuns(t *testing.T) { + open := windowed(t) + endsItsLine(t) + asksRecorded(t) + ctx := t.Context() + inv := open.inventory + if err := inv.RecordBuild(ctx, inventory.Build{ID: "build-one", Repository: "novox/one", Ref: "main", + Module: "one", Commit: "c0", On: "builder", Asked: t0.Add(-time.Hour), + Sources: []inventory.BuildSource{{Paths: []string{"main.go", "go.mod"}}}}); err != nil { + t.Fatal(err) + } + readme := repoMerge("one", "one1aaaaaaaa", t0) + readme.Paths = []string{"README.md"} + hear(t, open, readme, t0.Add(time.Second)) + cutAt(t, open, t0.Add(2*time.Minute)) + ws, _ := walks(t, open) + if len(ws) != 1 { + t.Fatalf("walks: %+v", ws) + } + w := ws[0] + if w.State != inventory.PlanDone || len(w.Tiers) != 0 || len(w.Modules) != 0 { + t.Fatalf("the walk reads %s, tiers %v, modules %v; want done with nothing to walk", w.State, w.Tiers, w.Modules) + } + if w.Delivery == nil || !strings.Contains(w.Delivery.Unchanged["one"], "holds none of the 1 changed file(s)") { + t.Fatalf("the walk does not say one matched what runs: %+v", w.Delivery) + } + if !strings.Contains(w.Note, "unchanged") || !strings.Contains(w.Note, "one") { + t.Errorf("the walk's note does not name it: %q", w.Note) + } + + // A merge that changes the build source is walked, and says nothing unchanged. + code := repoMerge("one", "one2bbbbbbbb", t0.Add(3*time.Minute)) + hear(t, open, code, t0.Add(3*time.Minute+time.Second)) + cutAt(t, open, t0.Add(6*time.Minute)) + ws, _ = walks(t, open) + if len(ws) != 2 { + t.Fatalf("walks: %+v", ws) + } + if _, built := ws[0].Modules["one"]; !built || ws[0].Delivery == nil || len(ws[0].Delivery.Unchanged) != 0 { + t.Fatalf("the walk of a change to main.go reads modules %v, delivery %+v", ws[0].Modules, ws[0].Delivery) + } +} + +func sortedOf(names []string) []string { + out := append([]string{}, names...) + sort.Strings(out) + return out +} diff --git a/cmd/mesh-controller/upgrades.go b/cmd/mesh-controller/upgrades.go index f5f57556..7f8bc8e2 100644 --- a/cmd/mesh-controller/upgrades.go +++ b/cmd/mesh-controller/upgrades.go @@ -692,9 +692,73 @@ func readsFile(e inventory.Entry, read []inventory.ReadRepository, p string) boo if own := ownSource(read); own != nil { return builder.SourceHolds(own, p) } + return readsDirectory(e, p) +} + +// readsDirectory is whether a changed file is in a module's directory, or anywhere for a module built from its +// repository's root: the reading of a module with no recorded build source. +func readsDirectory(e inventory.Entry, p string) bool { return strings.Trim(e.Source.Path, "/") == "" || inside(p, e.Source.Path) } +// unchangedOfMerge are the modules a merge changed files for under a reading without recorded build sources, but +// whose recorded build source holds none of them, each with why (novox/hq issue 611): not built, because the build +// the mesh holds is theirs (ADR 0267 §5). That reading — a delivery plan made while another walk built the module +// (issue 606) — would have moved them; the walk says they matched what runs rather than leaving them unnamed. +// A module built from the repository whose directory, or whole repository when built from its root, holds a +// changed file, and a module packaging the repository's branch, are each read so. A merge whose changed files +// were not all said moves every one of them: none is said here. Without acting. +func unchangedOfMerge(m link.SourceMoved, entries []inventory.Entry, + read map[string][]inventory.ReadRepository) map[string]string { + if len(m.Paths) == 0 || m.PathsTruncated { + return nil + } + from, _, _ := mergeCandidates(m, entries, read) + touched, _, _ := touchedBy(from, entries, m, read) + out := map[string]string{} + for _, e := range from { + name := e.Manifest.Module + if ownSource(read[name]) == nil || slices.ContainsFunc(touched, func(t inventory.Entry) bool { + return t.Manifest.Module == name + }) { + continue + } + var in []string + for _, p := range m.Paths { + if readsDirectory(e, p) { + in = append(in, p) + } + } + if len(in) > 0 { + out[name] = fmt.Sprintf("its build source holds none of the %d changed file(s) in its directory (e.g. %s): "+ + "the build the mesh holds is its build", len(in), in[0]) + } + } + for _, e := range entries { + name := e.Manifest.Module + if sourceIs(e.Source, m) || !packagesSaid(read[name], m) || readsFrom(read[name], m) { + continue + } + out[name] = fmt.Sprintf("its build source in %s/%s holds none of the %d changed file(s) (e.g. %s): "+ + "the build the mesh holds is its build", m.Owner, m.Repo, len(m.Paths), m.Paths[0]) + } + if len(out) == 0 { + return nil + } + return out +} + +// packagesSaid is whether a module's newest trunk build said the build source it read in a merge's repository and +// branch, as a context beside its own. +func packagesSaid(read []inventory.ReadRepository, m link.SourceMoved) bool { + for _, r := range read { + if !r.Own && sameRepository(r.Repository, m) && (r.Ref == "" || r.Ref == m.Base) && len(r.Paths) > 0 { + return true + } + } + return false +} + // staleIn is the modules whose recorded build source a plan has overtaken (novox/hq ADR 0267): a plan still // working that has yet to build one, or a plan made after that build which never built it — failed, stopped // or superseded. What such a module is built from is changing, or changed without a build to say so: a merge diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index d6933d04..f2293280 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -220,6 +220,11 @@ type PlanDelivery struct { // Withheld are, on a walk cut from a batch a stop walked merges again in, the modules it did not build: a // stopped merge moves them, and built only as dependents at the branch they would deliver it (ADR 0299). Withheld []string `json:"withheld,omitempty"` + // Unchanged are the modules a merge it answers changed files in the directory of, whose recorded build source + // holds none of them, each with why (novox/hq issue 611, ADR 0267 §5): not built, because the build the mesh + // holds is their build. A delivery plan that read them whole named them; this says they matched what runs, + // rather than that the walk left them unbuilt. Modules keeps meaning what the walk builds. + Unchanged map[string]string `json:"unchanged,omitempty"` // Batch is the window of a batch; nil once it is cut into a walk. Batch *PlanBatch `json:"batch,omitempty"` // Alone says the walk walks one merge on its own commit, after a failed walk carried it in a later one: diff --git a/testdata/store-tests b/testdata/store-tests index 20fe4b16..3e0e9706 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -44,3 +44,4 @@ ./cmd/mesh-controller TestReplay349 ./cmd/mesh-controller TestReplay348AFaultFromBeforeTheSendFailsNoGate ./cmd/mesh-controller TestReplay239AModuleNameIsNotTakenOverByAnotherRepository +./cmd/mesh-controller TestAWalkThatMovedNothingSaysWhichModulesMatchedWhatRuns