diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go index a631a5dd..fab99083 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go @@ -190,7 +190,7 @@ func (h *Holder) deliveryOf(repository string, number int, commit string) *check c.Check = &v } if g := h.groups[found.Group]; g != nil { - state, why := GroupState(g, h.membersOf(g)) + state, why, _ := h.groupState(g, h.membersOf(g)) c.groupSaid = state + ": " + why if g.Check != nil && g.Check.Verdict != "" { c.groupSaid += "; composed check " + g.Check.Verdict + " — " + g.Check.Summary diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index 193e4e68..ef4b0045 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -2217,15 +2217,22 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func( } } // Its state, said when it changes. - state, why := GroupState(g, members) + state, why, held := h.groupState(g, members) status := groupStatusState(state) + if held { + status = "pending" // no Approve exists while a member waits for its review (novox/hq ADR 0496 §2) + } if state == "delivering" && !(g.Check != nil && (g.Check.Verdict == "pass" || g.Check.Verdict == "warning") && sameMembers(g.Check.Commits, commitsOf(members)) && g.Check.OutOfDate == "") { // Success there lets the merge tool merge its members: only for a composed check that passed for these heads. status = "pending" } - if state != g.Said || status != g.SaidStatus { - g.Said, g.SaidStatus = state, status + reviewSaid := "" + if held { + reviewSaid = why + } + if state != g.Said || status != g.SaidStatus || reviewSaid != g.SaidReview { + g.Said, g.SaidStatus, g.SaidReview = state, status, reviewSaid for _, d := range members { g.Owed = append(g.Owed, Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery-group", State: status, Line: clip(fmt.Sprintf("group %s %s: %s", g.ID, state, why), 140), @@ -2719,6 +2726,29 @@ func GroupState(g *Group, members []*Delivery) (string, string) { return "checking", fmt.Sprintf("%d of %d member(s) ready", count[Ready], n) } +// groupState is GroupState, but a group whose members all passed and compose waits for a review while any member the +// Approve would cover has none at its head: its why names the members (novox/hq ADR 0496 §2, issue 1803), and held +// says its mesh/delivery-group is pending, not a success. Under the lock. +func (h *Holder) groupState(g *Group, members []*Delivery) (state, why string, held bool) { + state, why = GroupState(g, members) + if state != "ready" || g.Closed { + return state, why, false + } + var waiting []string + for _, m := range members { + if m.MergedAs != "" || m.State.Final() || h.covering(m) != nil { + continue + } + if _, ok := reviewWords(m); !ok { + waiting = append(waiting, "at "+shortOf(m.Commit, 12)+" ("+pullSaid(m)+")") + } + } + if len(waiting) == 0 { + return state, why, false + } + return state, waitsForReview + strings.Join(waiting, " and ")[len("at "):], true +} + // supersededWhy begins why a group whose members all ended is stopped. It is kept short: the forge's status line, // "group stopped: ", is clipped at 140 characters, and the member's id must fit in it whole. const supersededWhy = "superseded, not carried by a later delivery: " diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go index f9857f42..6204e918 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go @@ -363,6 +363,9 @@ func TestAGroupIsCheckedTogetherAndLetGoOnceEveryMemberIsMerged(t *testing.T) { ctl, cat := IDOf("novox/mesh-controller", ctlHead), IDOf("novox/mesh-catalog", catHead) w.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go")) w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) + // These heads are not full commit ids, so their description lines cannot name them: the reviews are set as read. + w.h.deliveries[ctl].ReviewedBy, w.h.deliveries[ctl].ReviewedAt = "tester@shanks", ctlHead + w.h.deliveries[cat].ReviewedBy, w.h.deliveries[cat].ReviewedAt = "tester@shanks", catHead if w.delivery(ctl).Group != "feat/x" || w.delivery(cat).Group != "feat/x" { t.Fatalf("one branch name in two repositories made no group: %q %q", w.delivery(ctl).Group, w.delivery(cat).Group) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go index b2abe5d5..7b966052 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go @@ -114,46 +114,110 @@ func TestA1803AnApproveAlreadyGivenIsNotAffected(t *testing.T) { } } -func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) { +const gTools, gHt = "novox/mesh-tools", "e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3" + +// groupOf is a green group of the given members (repository, number, head, review body), composed green, the operator not +// answering by himself. +func groupOf(t *testing.T, members [][4]string) *world { w := newWorld(t) w.op.auto = false - a := pr(gCtl, 1, gHc, "feat/g", "internal/a.go") - a.Body = "Reviewed-by: reviewer@shanks at " + gHc - b := pr(gCat, 2, gHk, "feat/g", "modules/app/module.json") - b.Body = "Reviewed-by: pending" - w.h.PullUpdated(a) - w.h.PullUpdated(b) - w.h.Checked(judged(gCtl, 1, gHc, "pass", gTrunk0)) - w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0)) + for i, m := range members { + p := pr(m[0], i+1, m[2], "feat/g", "src/a.go") + p.Body = m[3] + w.h.PullUpdated(p) + } + for i, m := range members { + w.h.Checked(judged(m[0], i+1, m[2], "pass", gTrunk0)) + } w.settleAll() g := w.h.groups["feat/g"] if g == nil || g.Check == nil { t.Fatalf("no composed check was asked: %+v", g) } - w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed", - Members: []struct { + c := CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed"} + for _, m := range members { + owner, repo, _ := strings.Cut(m[0], "/") + c.Members = append(c.Members, struct { Owner string `json:"owner"` Repo string `json:"repo"` Commit string `json:"commit"` Trunk string `json:"trunk,omitempty"` - }{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]}, - {Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gTrunk0[:12]}}}) + }{Owner: owner, Repo: repo, Commit: m[2], Trunk: gTrunk0[:12]}) + } + w.h.Checked(c) w.settleAll() + return w +} + +func reviewedAt(head string) string { return "Reviewed-by: reviewer@shanks at " + head } + +// wantGroupSays checks the group's state, its mesh/delivery-group status and the member's mesh/delivery status. +func (w *world) wantGroupSays(t *testing.T, label, member, head string, want, notWant string) { + t.Helper() + g := w.h.groups["feat/g"] + state, why, held := w.h.groupState(g, w.h.membersOf(g)) + if state != "ready" || !held || !strings.Contains(why, want) || (notWant != "" && strings.Contains(why, notWant)) { + t.Fatalf("%s: group state %s, why %q, held %v; wanted ready waiting with %q and without %q", label, state, why, held, want, notWant) + } + for _, c := range []string{"mesh/delivery-group", "mesh/delivery"} { + said := w.forge.statuses[member+"@"+head+" "+c] + if !strings.HasPrefix(said, "pending") || !strings.Contains(said, want) || (notWant != "" && strings.Contains(said, notWant)) { + t.Fatalf("%s: %s on %s says %q; wanted pending with %q and without %q", label, c, member, said, want, notWant) + } + } +} + +func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) { + w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}}) if w.op.count() != 0 { t.Fatalf("1803/AC5: a group with an unreviewed member was asked: %d asks", w.op.count()) } - for _, id := range []string{IDOf(gCtl, gHc), IDOf(gCat, gHk)} { - d := w.delivery(id) - if d.Ready == nil || !strings.Contains(d.Ready.Waits, "waits for a review at "+shortOf(gHk, 12)) || - !strings.Contains(d.Ready.Waits, "mesh-catalog#2") { - t.Fatalf("1803/AC5: %s waits %+v; wanted it to name mesh-catalog#2 and its head", id, d.Ready) - } - } + w.wantGroupSays(t, "1803/AC5", gCtl, gHc, "mesh-catalog#2", "mesh-controller#1") e := pr(gCat, 2, gHk, "feat/g") - e.Body = "Reviewed-by: reviewer@shanks at " + gHk + e.Body = reviewedAt(gHk) w.h.PullEdited(e) w.settleAll() if len(w.op.open()) != 1 { t.Fatalf("1803/AC5: every member reviewed, yet %d asks", w.op.count()) } + if said := w.forge.statuses[gCtl+"@"+gHc+" mesh/delivery-group"]; !strings.HasPrefix(said, "success") { + t.Fatalf("1803/AC5: the group's status once asked is %q", said) + } +} + +func TestA1803TheWaitingWordsFollowTheMembersStillWaiting(t *testing.T) { + w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}, {gTools, "", gHt, "Reviewed-by: pending"}}) + w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-catalog#2", "") + w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-tools#3", "") + e := pr(gCat, 2, gHk, "feat/g") + e.Body = reviewedAt(gHk) + w.h.PullEdited(e) + w.settleAll() + if w.op.count() != 0 { + t.Fatalf("1803/AC7: a member still waits, yet %d asks", w.op.count()) + } + // Only the other member is named now, on the group and on every member's own status. + w.wantGroupSays(t, "1803/AC7 one waiting", gCtl, gHc, "mesh-tools#3", "mesh-catalog#2") + w.wantGroupSays(t, "1803/AC7 one waiting", gCat, gHk, "mesh-tools#3", "mesh-catalog#2") +} + +func TestA1803APushAfterAnApprovingReviewHoldsTheNewHead(t *testing.T) { + w := greenWithBody(t, reviewedAt(readyHead)) + if len(w.op.open()) != 1 { + t.Fatalf("setup: the reviewed head is not asked") + } + // A new head on the same pull request whose description still names the old commit. + const pushed2 = "7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a" + p := pr("novox/app", 7, pushed2, "feat/x", "src/a.go") + p.Body = reviewedAt(readyHead) + w.h.PullUpdated(p) + w.h.Checked(judgedAt("novox/app", 7, pushed2, "pass", "t1t1t1t1t1t1")) + w.settleAll() + d := w.delivery(IDOf("novox/app", pushed2)) + if d.Ready == nil || !strings.Contains(d.Ready.Waits, waitsForReview+shortOf(pushed2, 12)) { + t.Fatalf("1803/AC8: the new head waits %+v; wanted it to wait for a review at %s", d.Ready, shortOf(pushed2, 12)) + } + if len(w.op.open()) != 0 { + t.Fatalf("1803/AC8: the new head was asked on the old review: %v", w.op.open()) + } } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index 77e0f19f..9425bfbe 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -1019,7 +1019,10 @@ type Group struct { Hold *GroupHold `json:"hold,omitempty"` // SaidStatus is the mesh/delivery-group status last written for it: a change of the status a state takes is // written too, though the state did not change (novox/hq issue 902). - SaidStatus string `json:"said_status,omitempty"` + SaidStatus string `json:"said_status,omitempty"` + // SaidReview is the wait for a review last said of the group while it was held for one (ADR 0496 §2): a change of + // the members still waiting is said too, though the state did not change. + SaidReview string `json:"said_review,omitempty"` Owed []Effect `json:"owed,omitempty"` } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index 8b461fd3..77737660 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -221,8 +221,8 @@ func (h *Holder) readyAsks(now time.Time) []func() { if waits != r.Waits { wasHeld := d.holdsForReview() r.Waits = waits - if wasHeld != d.holdsForReview() && d.shownOnItsPullRequest() { - // The status and the view say it waits for a review, and say no more once it is asked. + if (wasHeld || d.holdsForReview()) && d.shownOnItsPullRequest() { + // The status and the view say what it waits for, again whenever the words change while held, and no more once asked. state, desc := statusOf(d) d.owe(Effect{Kind: EffectView, Since: now}) d.owe(Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery", State: state, Line: desc, Since: now}) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go index 8e6c747c..3545cfcd 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go @@ -134,14 +134,14 @@ func (h *Holder) Show(id string) (any, error) { out["reads"] = d.strandedEndedSaid() // stopped, and nothing can carry what it waited for (issue 1581) } if g := h.groups[d.Group]; g != nil { - state, why := GroupState(g, h.membersOf(g)) + state, why, _ := h.groupState(g, h.membersOf(g)) out["group"] = map[string]any{"group": g, "state": state, "why": why} } return out, nil } if g := h.groups[id]; g != nil { members := h.membersOf(g) - state, why := GroupState(g, members) + state, why, _ := h.groupState(g, members) return map[string]any{"group": g, "state": state, "why": why, "members": members}, nil } return nil, fmt.Errorf("no delivery or group %q: `deliveries` lists them", id) @@ -184,7 +184,7 @@ func (h *Holder) Groups(all bool) []GroupLine { if len(members) == 0 { continue } - state, why := GroupState(g, members) + state, why, _ := h.groupState(g, members) if !all && (state == "delivered" || state == "failed" || state == "stopped") { continue }