From 07b050790ea2840a9df71b42be2334a9c35a09c9 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 17 Oct 2026 21:37:37 +0200 Subject: [PATCH] Review of issue 977: keep the plan's own order for its gates, hold a group only for a composed mesh-delivery, keep mark-ready one release, compare with the batch's start, fail a lone cycle (issue 977) --- cmd/mesh-controller/batches.go | 27 +++++- cmd/mesh-controller/batches_test.go | 9 +- cmd/mesh-controller/checks.go | 13 +++ cmd/mesh-controller/composed_verdict_test.go | 4 +- cmd/mesh-controller/delivery.go | 15 +-- cmd/mesh-controller/delivery_changes.go | 29 +++++- cmd/mesh-controller/delivery_edges_test.go | 97 ++++++++++++++++++++ cmd/mesh-controller/group_one_batch_test.go | 16 ++++ cmd/mesh-controller/group_wait.go | 22 +++-- cmd/mesh-controller/release_plan.go | 4 + internal/catalogue/delivery_seat.go | 11 +++ internal/catalogue/delivery_seat_test.go | 29 ++++++ internal/inventory/plans.go | 4 + internal/link/build.go | 3 +- 14 files changed, 259 insertions(+), 24 deletions(-) diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index beee8b63..96235aa1 100644 --- a/cmd/mesh-controller/batches.go +++ b/cmd/mesh-controller/batches.go @@ -896,7 +896,7 @@ func releaseHolds(ctx context.Context, inv *inventory.Inventory, batches []inven } changed := false for head := range b.Delivery.Batch.Holds { - open, isGroup, err := openMembersOf(ctx, inv, deliverySeatHeld(entries), head, "") + open, isGroup, _, err := openMembersOf(ctx, inv, deliverySeatHeld(entries), head, "") if err != nil || (isGroup && len(open) > 0) { continue } @@ -1139,7 +1139,7 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ } } sort.Strings(also) - ordered, err := changeOrderEdges(ctx, members, moved, edges) + ordered, err := changeOrderEdges(ctx, merges, members, entries, moved, edges) if err != nil { return err } @@ -1167,6 +1167,7 @@ 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.Order = ordered // **Left where it is, a module whose program did not change** (novox/hq issue 721): its check found it the same as // the build the mesh holds. for n, why := range leaveSameAsRunning(ctx, inv, &plan, moved, append(append([]inventory.Edge{}, edges...), ordered...)) { @@ -1443,11 +1444,27 @@ type batchMember struct { // whatever repository the merges are in and whether or not they form a delivery group. No `after:` line is read. // A cycle is said, never lost: the plan puts the modules left in its last tier, and the composed check fails the // group before it merges. -func changeOrderEdges(ctx context.Context, members []batchMember, moved []string, edges []inventory.Edge) ([]inventory.Edge, error) { +func changeOrderEdges(ctx context.Context, merges []inventory.BatchedMerge, members []batchMember, + entries []inventory.Entry, moved []string, edges []inventory.Edge) ([]inventory.Edge, error) { + // What each repository's branch held before the batch: the first parent of its earliest merge in it. + earliest := map[string]inventory.BatchedMerge{} + for _, k := range merges { + key := strings.ToLower(k.Repository) + "\x00" + k.Branch + if e, ok := earliest[key]; !ok || laterMerge(e, k) { + earliest[key] = k + } + } var sources []changeSource for _, m := range members { - sources = append(sources, changeSource{Owner: m.merge.Owner, Repo: m.merge.Repo, New: m.merge.Commit, - Paths: m.merge.Paths, Truncated: m.merge.PathsTruncated, Removed: m.merge.Removed}) + s := changeSource{Owner: m.merge.Owner, Repo: m.merge.Repo, New: m.merge.Commit, + Paths: m.merge.Paths, Truncated: m.merge.PathsTruncated, Removed: m.merge.Removed} + if e, ok := earliest[strings.ToLower(m.merge.Owner+"/"+m.merge.Repo)+"\x00"+m.merge.Base]; ok { + s.Since = e.Commit + } + if s.Truncated { + s.Paths, s.Truncated = truncatedPaths(entries, s.Owner, s.Repo, moved), false + } + sources = append(sources, s) } changes, err := changesOf(ctx, sources) if err != nil { diff --git a/cmd/mesh-controller/batches_test.go b/cmd/mesh-controller/batches_test.go index 90a5ac8a..a5568700 100644 --- a/cmd/mesh-controller/batches_test.go +++ b/cmd/mesh-controller/batches_test.go @@ -122,6 +122,13 @@ func everyHeadWaits(t *testing.T) { // groupStates stubs mesh-delivery's word on one group: its members and their states, which a test changes as the // members merge. func groupStates(t *testing.T, head string, states map[string]string) { + t.Helper() + groupStatesFrom(t, head, states, true) +} + +// groupStatesFrom is groupStates for a mesh-delivery that does (composed) or does not yet say the group's gate is the +// composed check's: only the first holds the group's batch open for its last member. +func groupStatesFrom(t *testing.T, head string, states map[string]string, composed bool) { t.Helper() was := deliveryGroupOf deliveryGroupOf = func(_ context.Context, _ bool, asked string) ([]groupMember, bool, error) { @@ -130,7 +137,7 @@ func groupStates(t *testing.T, head string, states map[string]string) { } var members []groupMember for repository, state := range states { - members = append(members, groupMember{Repository: repository, State: state}) + members = append(members, groupMember{Repository: repository, State: state, Composed: composed}) } return members, true, nil } diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index e20f7737..b564f0c7 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -154,6 +154,19 @@ func (f following) PullUpdated(ctx context.Context, p link.PullUpdated) error { return err } scope := pullScope(p, entries, read, edges) + // **A cycle of edges fails the check, naming every module and edge** (novox/hq ADR 0419 clause 6), a lone pull + // request's as a group's composed check is: no order exists for what it changes. + alone := orderMember{ID: p.Owner + "/" + p.Repo + "@" + twelve(p.Commit), Repository: p.Owner + "/" + p.Repo, Base: p.Base, + Head: p.Commit, Number: p.Number, Paths: p.Paths, PathsTruncated: p.PathsTruncated, Removed: p.Removed, BaseSHA: p.BaseSHA} + if failing, cyclic, err := cycleVerdict(ctx, "", []orderMember{alone}, + map[string]orderReach{alone.ID: {Moved: scope.Modules}}, edges, entries); err != nil { + return err + } else if cyclic { + failing.Commit, failing.Trunk = p.Commit, twelve(p.BaseSHA) + fmt.Printf("%s/%s#%d (%.8s): %s\n", p.Owner, p.Repo, p.Number, p.Commit, failing.Summary) + sayChecked(ctx, failing) + return nil + } // **A member of a group whose gate is the composed check's runs its repository's check alone** (novox/hq ADR 0403 // clause 1): mesh-delivery is asked once, and says so on the group's line. Told nothing, the pull request is // checked alone, gate included (clause 6). diff --git a/cmd/mesh-controller/composed_verdict_test.go b/cmd/mesh-controller/composed_verdict_test.go index f85b9850..a8573833 100644 --- a/cmd/mesh-controller/composed_verdict_test.go +++ b/cmd/mesh-controller/composed_verdict_test.go @@ -111,7 +111,7 @@ func TestAGroupInACycleFailsItsComposedCheckWithoutABuild(t *testing.T) { return catalogue.Manifest{Module: "my", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "b"}}}}, Invokes: []string{"seat:s.a"}}, true, nil } - c, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, []inventory.Edge(nil)) + c, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, []inventory.Edge(nil), nil) if err != nil || !cyclic { t.Fatalf("no cycle found: %v", err) } @@ -130,7 +130,7 @@ func TestAGroupInACycleFailsItsComposedCheckWithoutABuild(t *testing.T) { manifestAt = func(context.Context, string, string, string, string) (catalogue.Manifest, bool, error) { return catalogue.Manifest{}, false, nil } - if _, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, nil); err != nil || cyclic { + if _, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, nil, nil); err != nil || cyclic { t.Fatalf("a cycle where there is none: %v %v", cyclic, err) } } diff --git a/cmd/mesh-controller/delivery.go b/cmd/mesh-controller/delivery.go index 3fc13979..e8a8da77 100644 --- a/cmd/mesh-controller/delivery.go +++ b/cmd/mesh-controller/delivery.go @@ -582,8 +582,8 @@ type orderMember struct { // BaseSHA is the base branch's head the forge last announced the head with (novox/hq ADR 0323): the trunk commit // a verdict no build judges names when the head is checked again through mesh-delivery. BaseSHA string `json:"base_sha,omitempty"` - // Ready says the member's pull request is marked ready (novox/hq ADR 0406 §3, issue 974): its check is queued before - // every other check. Said by mesh-delivery on a ready mark. + // Ready says mesh-delivery wants the member's check queued before every other check (novox/hq ADR 0406 §3, issue + // 974). There is no ready mark any more (issue 1022): mesh-delivery says it when it asks for the Approve. Ready bool `json:"ready,omitempty"` } @@ -828,7 +828,7 @@ func deliveryCommand(ctx context.Context, args []string) error { if err != nil { return err } - changes, err := changesOf(ctx, changeSourcesOf(members)) + changes, err := changesOf(ctx, changeSourcesOf(members, entries, reachOfMembers(members, entries, read, edges))) if err != nil { return fmt.Errorf("what the members say of their manifests cannot be read: %w", err) } @@ -1018,8 +1018,8 @@ func theGraph(ctx context.Context, inv *inventory.Inventory) ([]inventory.Entry, // group's composed check, said without a build. The manifests' change is read from the forge; an answer it cannot give // is an error, never read as no cycle. func cycleVerdict(ctx context.Context, group string, members []orderMember, reach map[string]orderReach, - edges []inventory.Edge) (link.Checked, bool, error) { - changes, err := changesOf(ctx, changeSourcesOf(members)) + edges []inventory.Edge, entries []inventory.Entry) (link.Checked, bool, error) { + changes, err := changesOf(ctx, changeSourcesOf(members, entries, reach)) if err != nil { return link.Checked{}, false, fmt.Errorf("what the group's members say of their manifests cannot be read: %w", err) } @@ -1036,6 +1036,9 @@ func cycleVerdict(ctx context.Context, group string, members []orderMember, reac Commit: first.Commit, Verdict: "fail", Summary: said, Report: said, Group: group, Gate: &link.CheckLayer{Verdict: "fail", Summary: said}} for _, m := range members { + if group == "" { + break // a lone pull request's verdict names no group and no member + } p := m.pull() c.Members = append(c.Members, link.CheckedMember{Owner: p.Owner, Repo: p.Repo, Number: p.Number, Commit: p.Commit, Trunk: twelve(p.BaseSHA)}) @@ -1075,7 +1078,7 @@ func askGroupCheck(ctx context.Context, open *stores, group string, members []or reach := reachOfMembers(members, entries, read, edges) // **A cycle of edges fails the group's composed check, naming every module and edge** (novox/hq ADR 0419 clause 6), // before the build seat is asked: no composition of the heads has an order to be sent in. - if failing, cyclic, err := cycleVerdict(ctx, group, members, reach, edges); err != nil { + if failing, cyclic, err := cycleVerdict(ctx, group, members, reach, edges, entries); err != nil { return "", err } else if cyclic { sayChecked(ctx, failing) diff --git a/cmd/mesh-controller/delivery_changes.go b/cmd/mesh-controller/delivery_changes.go index 9c7d52ff..2fb5f82f 100644 --- a/cmd/mesh-controller/delivery_changes.go +++ b/cmd/mesh-controller/delivery_changes.go @@ -13,6 +13,7 @@ import ( "time" "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" ) @@ -28,6 +29,9 @@ type changeSource struct { Paths []string Truncated bool Removed []string + // Since is the first commit of the repository in the change, when the change is several merges: what it replaced + // is the first parent of that one, so a verb an earlier merge added is added by the change too. + Since string } // manifestAt reads a repository's module.json at a ref from the forge; the bool says it exists there. A variable so a @@ -206,7 +210,11 @@ func changesOf(ctx context.Context, sources []changeSource) (map[string]moduleCh } old := s.Old if old == "" { - if old, err = parentOf(ctx, s.Owner, s.Repo, s.New); err != nil { + first := s.Since + if first == "" { + first = s.New + } + if old, err = parentOf(ctx, s.Owner, s.Repo, first); err != nil { return nil, err } } @@ -230,11 +238,28 @@ func changesOf(ctx context.Context, sources []changeSource) (map[string]moduleCh return out, nil } +// truncatedPaths are the manifests a change that names only a beginning of its files is read as changing: those of the +// modules it moves that are built from its repository. Said, never skipped: no edge may be lost to a long list. +func truncatedPaths(entries []inventory.Entry, owner, repo string, moved []string) []string { + var out []string + for _, e := range entries { + if slices.Contains(moved, e.Manifest.Module) && sameRepository(e.Source.Repository, link.SourceMoved{Owner: owner, Repo: repo}) { + out = append(out, path.Join(strings.Trim(e.Source.Path, "/"), moduleManifestFile)) + } + } + fmt.Printf(" %s/%s changed more files than were listed: the manifests of its modules (%s) are read as changed\n", + owner, repo, strings.Join(out, ", ")) + return out +} + // changeSourcesOf are a group's members as changes to read: each head over what its pull request merges into. -func changeSourcesOf(members []orderMember) []changeSource { +func changeSourcesOf(members []orderMember, entries []inventory.Entry, reach map[string]orderReach) []changeSource { var out []changeSource for _, m := range members { p := m.pull() + if p.PathsTruncated { + p.Paths, p.PathsTruncated = truncatedPaths(entries, p.Owner, p.Repo, reach[m.ID].Moved), false + } old := p.BaseSHA if old == "" { old = p.Base diff --git a/cmd/mesh-controller/delivery_edges_test.go b/cmd/mesh-controller/delivery_edges_test.go index 519b4fb7..195aa2eb 100644 --- a/cmd/mesh-controller/delivery_edges_test.go +++ b/cmd/mesh-controller/delivery_edges_test.go @@ -1,9 +1,11 @@ package main import ( + "context" "strings" "testing" + "github.com/novox/mesh-controller/internal/catalogue" "github.com/novox/mesh-controller/internal/inventory" ) @@ -113,3 +115,98 @@ func TestTheOrderIsPerModuleNotPerRepository(t *testing.T) { t.Fatalf("tiers %v, want [a b] then [c]", tiers) } } + +// The edges a plan's order was drawn with are kept on the plan, and its gates read them: a later tier waits for the +// earlier to be applied although the catalogue's graph holds no such edge (ADR 0419, the lead's settlement Q4). +func TestAPlansOwnOrderGatesItsTiersWhateverTheStoreGraphHolds(t *testing.T) { + p := inventory.Plan{Tiers: [][]string{{"server"}, {"caller"}}, Tier: 0, + Delivery: &inventory.PlanDelivery{Order: []inventory.Edge{{From: "caller", To: "server", Kind: edgeChangeOrder, + Why: ruleVerbBeforeCaller + ": s.v"}}}} + if got := gates(p, nil, func(string) bool { return true }); len(got) != 1 || got[0] != "server" { + t.Fatalf("the caller's tier waits for %v; want the server", got) + } + p.Delivery.Order = nil + if got := gates(p, nil, func(string) bool { return true }); len(got) != 0 { + t.Fatalf("a plan with no order waits for %v", got) + } +} + +// A verb an earlier merge of the same repository in the batch added is added by the batch: the change is compared with +// what the repository held before the batch, not with the first parent of its latest merge. +func TestAVerbAnEarlierMergeOfTheBatchAddedDrawsItsEdge(t *testing.T) { + was, wasParent := manifestAt, parentOf + t.Cleanup(func() { manifestAt, parentOf = was, wasParent }) + manifestsReadMu.Lock() + clear(manifestsRead) + manifestsReadMu.Unlock() + const first, second, before = "1111111111111111111111111111111111111111", "2222222222222222222222222222222222222222", "0000000000000000000000000000000000000000" + parentOf = func(_ context.Context, _, _, commit string) (string, error) { + if commit == first { + return before, nil + } + return first, nil // the latest merge's parent holds the verb already + } + manifestAt = func(_ context.Context, _, _, _, ref string) (catalogue.Manifest, bool, error) { + m := catalogue.Manifest{Module: "server"} + if ref != before { + m.DefinesSeats = []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "v"}}}} + } + return m, true, nil + } + src := changeSource{Owner: "novox", Repo: "r", New: second, Paths: []string{"module.json"}} + if got, err := changesOf(context.Background(), []changeSource{src}); err != nil || len(got["server"].Serves) != 0 { + t.Fatalf("against the first parent of the latest merge: %+v %v", got, err) + } + src.Since = first + got, err := changesOf(context.Background(), []changeSource{src}) + if err != nil || len(got["server"].Serves) != 1 || got["server"].Serves[0] != "s.v" { + t.Fatalf("against what the batch found: %+v %v", got, err) + } +} + +// A change that names only a beginning of its files is read as changing the manifests of the modules it moves. +func TestATruncatedChangeIsReadAsChangingTheManifestsOfWhatItMoves(t *testing.T) { + entries := []inventory.Entry{ + {Manifest: catalogue.Manifest{Module: "a"}, Source: inventory.Source{Repository: "novox/mesh-catalog", Path: "modules/a"}}, + {Manifest: catalogue.Manifest{Module: "b"}, Source: inventory.Source{Repository: "novox/mesh-catalog", Path: "modules/b"}}, + {Manifest: catalogue.Manifest{Module: "c"}, Source: inventory.Source{Repository: "novox/other", Path: "modules/c"}}, + } + got := truncatedPaths(entries, "novox", "mesh-catalog", []string{"a", "c"}) + if len(got) != 1 || got[0] != "modules/a/module.json" { + t.Fatalf("read as changed: %v", got) + } + members := []orderMember{{ID: "m", Repository: "novox/mesh-catalog", Head: "h", PathsTruncated: true, Paths: []string{"x"}}} + src := changeSourcesOf(members, entries, map[string]orderReach{"m": {Moved: []string{"a"}}}) + if len(src) != 1 || src[0].Truncated || len(src[0].Paths) != 1 || src[0].Paths[0] != "modules/a/module.json" { + t.Fatalf("the member's change is read as %+v", src) + } +} + +// A lone pull request's cycle fails its check, naming every module and edge, with no group and no members. +func TestALonePullRequestInACycleFailsWithNoGroup(t *testing.T) { + was, wasParent := manifestAt, parentOf + t.Cleanup(func() { manifestAt, parentOf = was, wasParent }) + manifestsReadMu.Lock() + clear(manifestsRead) + manifestsReadMu.Unlock() + const head = "3333333333333333333333333333333333333333" + parentOf = func(context.Context, string, string, string) (string, error) { return "", nil } + manifestAt = func(_ context.Context, _, _, file, ref string) (catalogue.Manifest, bool, error) { + if ref == "main" { + return catalogue.Manifest{}, false, nil + } + if file == "modules/x/module.json" { + return catalogue.Manifest{Module: "mx", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "a"}}}}, + Invokes: []string{"seat:s.b"}}, true, nil + } + return catalogue.Manifest{Module: "my", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "b"}}}}, + Invokes: []string{"seat:s.a"}}, true, nil + } + m := orderMember{ID: "novox/r@" + head[:12], Repository: "novox/r", Base: "main", Head: head, Number: 5, + Paths: []string{"modules/x/module.json", "modules/y/module.json"}} + c, cyclic, err := cycleVerdict(context.Background(), "", []orderMember{m}, map[string]orderReach{m.ID: {Moved: []string{"mx", "my"}}}, nil, nil) + if err != nil || !cyclic || c.Verdict != "fail" || c.Group != "" || len(c.Members) != 0 || !strings.Contains(c.Summary, "s.a") || + !strings.Contains(c.Summary, "my") { + t.Fatalf("the verdict is %+v (cyclic %v, %v)", c, cyclic, err) + } +} diff --git a/cmd/mesh-controller/group_one_batch_test.go b/cmd/mesh-controller/group_one_batch_test.go index a4a55025..b7c69e57 100644 --- a/cmd/mesh-controller/group_one_batch_test.go +++ b/cmd/mesh-controller/group_one_batch_test.go @@ -121,3 +121,19 @@ func TestAHeldBatchSurvivesARestart(t *testing.T) { t.Fatalf("after an hour: %d walk(s), batches %+v", len(ws), bs) } } + +// A mesh-delivery that does not yet say the group's gate is composed merges member two only after member one is +// delivered, so a batch held for it would wait for ever: the merge waits for the window, as before. +func TestAnOlderMeshDeliverysGroupIsNotHeldOpen(t *testing.T) { + open := windowed(t) + asksWithPaths(t) + groupStatesFrom(t, "feat/g", map[string]string{"novox/one": "ready", "novox/two": "ready"}, false) + hear(t, open, fromHead(repoMerge("one", "c1", t0), "feat/g"), t0) + if _, bs := walks(t, open); len(bs) != 1 || len(bs[0].Delivery.Batch.Holds) != 0 { + t.Fatalf("held for an older mesh-delivery's group: %+v", bs) + } + cutAt(t, open, t0.Add(90*time.Second)) + if ws, bs := walks(t, open); len(ws) != 1 || len(bs) != 0 { + t.Fatalf("at its window: %d walk(s), %d batch(es)", len(ws), len(bs)) + } +} diff --git a/cmd/mesh-controller/group_wait.go b/cmd/mesh-controller/group_wait.go index c8a62f3f..46a22e03 100644 --- a/cmd/mesh-controller/group_wait.go +++ b/cmd/mesh-controller/group_wait.go @@ -35,6 +35,10 @@ var openPullStates = map[string]bool{"proposed": true, "checked": true, "ready": // groupMember is one member of a delivery group as mesh-delivery's `groups` says it: `@ (state)`. type groupMember struct { Repository, State string + // Composed says the group's line says its gate is the composed check's (a mesh-delivery that merges a group in + // one pass): only such a group is held open for its last member, since an older mesh-delivery merges member two + // only after member one is delivered, and a held batch would wait for ever. + Composed bool } // groupsLine is one group as mesh-delivery's `groups` says it: its id (the head branch name), its members as text and, @@ -103,7 +107,9 @@ var deliveryGroupOf = func(ctx context.Context, held bool, head string) ([]group } var members []groupMember for _, s := range l.Members { - members = append(members, memberOf(s)) + m := memberOf(s) + m.Composed = l.ComposedGate + members = append(members, m) } return members, true, nil } @@ -140,7 +146,7 @@ func groupWaitHolding(ctx context.Context, inv *inventory.Inventory, m link.Sour if m.Head == "" { return "", false, nil } - open, isGroup, err := openMembersOf(ctx, inv, held, m.Head, m.Owner+"/"+m.Repo) + open, isGroup, composed, err := openMembersOf(ctx, inv, held, m.Head, m.Owner+"/"+m.Repo) if err != nil { return fmt.Sprintf("%s did not say whether group %s has a member still to merge (%v): it waits for the "+ "window, the safe side", catalogue.DeliverySeat, m.Head, err), false, nil @@ -148,29 +154,31 @@ func groupWaitHolding(ctx context.Context, inv *inventory.Inventory, m link.Sour if !isGroup || len(open) == 0 { return "", false, nil } - return "group " + m.Head + " has a member still to merge: " + strings.Join(open, ", "), true, nil + return "group " + m.Head + " has a member still to merge: " + strings.Join(open, ", "), composed, nil } // openMembersOf are the members of the group a head names that are still to merge, in words: those whose pull // request is open and whose merge no batch not yet cut keeps. self is a repository to leave out, the merge being heard. -func openMembersOf(ctx context.Context, inv *inventory.Inventory, held bool, head, self string) ([]string, bool, error) { +func openMembersOf(ctx context.Context, inv *inventory.Inventory, held bool, head, self string) ([]string, bool, bool, error) { kept, err := keptFromHead(ctx, inv, head) if err != nil { - return nil, false, err + return nil, false, false, err } members, isGroup, err := deliveryGroupOf(ctx, held, head) if err != nil || !isGroup { - return nil, isGroup, err + return nil, isGroup, false, err } var open []string + composed := false for _, g := range members { + composed = composed || g.Composed if strings.EqualFold(g.Repository, self) || kept[strings.ToLower(g.Repository)] || !openPullStates[g.State] { continue } open = append(open, g.Repository+" ("+g.State+")") } sort.Strings(open) - return open, true, nil + return open, true, composed, nil } // keptFromHead are the repositories (lower case) of the merges from a head that batches not yet cut hold, of diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 71373a8a..69aa1f8d 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -200,6 +200,10 @@ func gates(p inventory.Plan, edges []inventory.Edge, rollsOut func(string) bool) } seen := map[string]bool{} var out []string + // The plan's own order (ADR 0419) is not in the catalogue's graph: it is kept on the plan. + if p.Delivery != nil { + edges = append(append([]inventory.Edge{}, edges...), p.Delivery.Order...) + } for _, e := range edges { // A delivery group's order inside a walk (novox/hq ADR 0276 decision 5) gates as built-by does: the // member before is running on its machines before the member after is asked. diff --git a/internal/catalogue/delivery_seat.go b/internal/catalogue/delivery_seat.go index 840c4916..dbc80faf 100644 --- a/internal/catalogue/delivery_seat.go +++ b/internal/catalogue/delivery_seat.go @@ -84,6 +84,17 @@ func deliveryVerbs() []Verb { "dry": "\"true\": what it would retire, changing nothing"}, []string{"why"}, "dry"), // Added after the seat's holder shipped (novox/hq issue 313): optional until mesh-delivery serves it. Optional: true}, + {Name: "mark-ready", Description: "A reviewer's word that a delivery's head is approved: the holder puts the " + + "label mesh/ready on its pull request and then writes \"ready at \", the order the forge counts, " + + "as the mesh's own forge account, naming who reviewed it and why. Refused when the delivery's commit is " + + "no longer its pull request's head. mesh-delivery then merges it once its checks pass (novox/hq ADR 0364).", + Input: schema(map[string]string{"id": "the delivery's id (owner/repository@commit): the head reviewed", + "reviewed-by": "who reviewed it, as who@node", "why": "the review's verdict in a line"}, + []string{"id", "reviewed-by", "why"}), + // Kept one more release, optional (novox/hq issue 1022): mesh-delivery no longer serves it, but the manifest + // it still has on the mesh does until its catalogue member merges, and a seat refuses a holder that serves + // a verb it does not promise. The next release removes this entry. + Optional: true}, } } diff --git a/internal/catalogue/delivery_seat_test.go b/internal/catalogue/delivery_seat_test.go index 5fe8daf2..7d3a6193 100644 --- a/internal/catalogue/delivery_seat_test.go +++ b/internal/catalogue/delivery_seat_test.go @@ -133,3 +133,32 @@ func TestTheDeliverySeatPromisesTimesOptionally(t *testing.T) { t.Fatalf("a holder serving times: %v", err) } } + +// The seat still promises `mark-ready`, optionally, for one more release (issue 1022); it was added by novox/hq issue 913: `mark-ready`, with the delivery's id, who reviewed it +// and why, so a reviewer marks a pull request through one audited verb, label and comment in the order the forge +// counts, instead of two raw forge calls. Added after the holder shipped, it is optional. +func TestTheDeliverySeatPromisesMarkReadyOptionally(t *testing.T) { + seat, _ := SeatNamed(DeliverySeat) + var mark *Verb + for i := range seat.Serves { + if seat.Serves[i].Name == "mark-ready" { + mark = &seat.Serves[i] + } + } + if mark == nil || !mark.Optional { + t.Fatalf("the delivery seat promises %v, mark-ready optionally", VerbNames(seat.Serves)) + } + if req, _ := mark.Input["required"].([]string); !reflect.DeepEqual(req, []string{"id", "reviewed-by", "why"}) { + t.Errorf("mark-ready requires %v, wanted id, reviewed-by and why", req) + } + var without []string + for _, v := range VerbNames(seat.Serves) { + if v != "mark-ready" { + without = append(without, v) + } + } + m := Manifest{Module: "mesh-delivery", Claims: []Claim{{Name: DeliverySeat, Scope: ScopeMesh, Serves: without}}} + if err := CanHold(m, seat); err != nil { + t.Fatalf("a holder without mark-ready yet: %v", err) + } +} diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index ae7e9fb9..c2e2bed4 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -205,6 +205,10 @@ func (p Plan) Batch() bool { return p.State == PlanAssembling || p.State == Plan // PlanDelivery is what a walk waits for and what came of the wait (novox/hq ADR 0239). type PlanDelivery struct { + // Order are the edges the plan's own order was drawn with (novox/hq ADR 0419): verb before caller, version skew, + // engine before controller. Kept on the plan because the catalogue's graph does not hold them, and the plan's gates + // (a later tier waits for the earlier to be applied) are read from them. + Order []Edge `json:"order,omitempty"` // Awaits is who must say the walk may start: the seat whose holder owns the delivery. Awaits string `json:"awaits"` // Go is when it was let go, By by whom (the seat's holder, or a person's name) and Why. diff --git a/internal/link/build.go b/internal/link/build.go index ffccf720..d40ca40a 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -148,7 +148,8 @@ type CheckRequest struct { // 0406 §4): with the head, what the pull request's key says the current check is for, and what a waiting request // is dropped by and a running check stopped by when the key moves. Trunk string `json:"trunk,omitempty"` - // Ready says the pull request is marked ready (ADR 0406 §3): its check is queued before every other check. + // Ready says mesh-delivery wants this check queued before every other check (ADR 0406 §3); there is no ready mark + // any more (issue 1022). Ready bool `json:"ready,omitempty"` // Composed says the pull request is a member of a delivery group whose gate is the composed check's (novox/hq ADR // 0403 clause 1): this check runs the repository's own merge-check.sh alone, and its gate layer is not run.