diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go index e2638bfc..8c17bd12 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go @@ -163,3 +163,85 @@ func TestAnyOtherRefusalIsStalledAtOnce(t *testing.T) { t.Fatalf("a missing status refusal is not said at once: %+v", got) } } + +// rejectedAfterItsApprove is a delivery approved and then rejected: its check failed after the ask (as in events_test.go), +// its pull request's description naming who pushed it when pushedBy is given. +func rejectedAfterItsApprove(t *testing.T, pushedBy string) (*world, string) { + t.Helper() + w := newWorld(t) + w.op.auto = false + p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") + if pushedBy != "" { + p.Body += "\nPushed-by: " + pushedBy + } + w.h.PullUpdated(p) + w.settleAll() + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + if _, err := w.h.Recheck(id, "a second look", "tester"); err != nil { + t.Fatal(err) + } + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "fail", "t1t1t1t1t1t1")) + approve(w) + w.h.React() + if w.state(id) != Rejected { + t.Fatalf("not rejected: %s", w.state(id)) + } + return w, id +} + +// ADR 0505 §5: a rejected delivery is told on its pull request at once, and reaches the operator only after two hours +// with no new head, in words naming its author (its branch, and the session that pushed it) and needing a new head — +// never "stop it", never "the operator's". +func TestARejectedDeliveryIsItsAuthorsForTwoHours(t *testing.T) { + w, id := rejectedAfterItsApprove(t, "quiet-deliveries@shanks") + said := false + for _, c := range w.forge.comments { + said = said || strings.Contains(c, "not merged") + } + if !said { + t.Fatalf("its author was not told on the pull request at once: %v", w.forge.comments) + } + w.later(2*time.Hour - time.Minute) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("the operator's before two hours: %+v", got) + } + w.later(2 * time.Minute) + got := linesFor(w, id) + if len(got) != 1 { + t.Fatalf("not the operator's after two hours with no new head: %+v", got) + } + l := got[0] + if l.Needs != NeedsNewHead || l.Author != "the author of feat/x (quiet-deliveries on shanks)" || l.Branch != "feat/x" { + t.Fatalf("the line does not name its author and the new head it needs: %+v", l) + } + for _, never := range []string{"operator's", "stop it"} { + if strings.Contains(l.Says+l.H2, never) { + t.Fatalf("the line says %q: %+v", never, l) + } + } + if !strings.Contains(l.Says, "needs a new head from the author of feat/x (quiet-deliveries on shanks)") { + t.Fatalf("the words do not say a new head is needed from its author: %q", l.Says) + } +} + +// ADR 0505 §5: with no Pushed-by line, the branch alone names its author. +func TestARejectedDeliveryWithNoPushedByLineIsNamedByItsBranch(t *testing.T) { + w, id := rejectedAfterItsApprove(t, "") + w.later(2*time.Hour + time.Minute) + if got := linesFor(w, id); len(got) != 1 || got[0].Author != "the author of feat/x" { + t.Fatalf("not named by its branch: %+v", got) + } +} + +// ADR 0505 §5: a new head within the two hours ends it: the rejected head is never the operator's. +func TestANewHeadWithinTwoHoursKeepsTheRejectedHeadFromTheOperator(t *testing.T) { + w, id := rejectedAfterItsApprove(t, "quiet-deliveries@shanks") + w.later(time.Hour) + w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) + w.settleAll() + w.later(2 * time.Hour) + if got := linesFor(w, id); len(got) != 0 { + t.Fatalf("the superseded head reached the operator: %+v", got) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go index 90e2d95c..f5e1f299 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "strings" "testing" + "time" stdio "git.novox.be/novox/mesh-sdk/go" ) @@ -229,7 +230,8 @@ func TestAPushAfterTheAskEndsItAndSaysSoOnceOnThePullRequest(t *testing.T) { } } -// Issue 1022 (ADR 0421 §7): a check that failed after the ask merges nothing and is said once and as a stalled line. +// Issue 1022 (ADR 0421 §7), as novox/hq ADR 0505 §5 changed it: a check that failed after the ask merges nothing, is +// said once on the pull request at once, and is a stalled line only after two hours with no new head. func TestACheckThatFailsAfterTheAskIsNotMergedAndIsSaid(t *testing.T) { w := announced(t) id := IDOf("novox/app", readyHead) @@ -252,6 +254,10 @@ func TestACheckThatFailsAfterTheAskIsNotMergedAndIsSaid(t *testing.T) { if said != 1 { t.Fatalf("the failure is not said once: %v", w.forge.comments) } + if lines := w.h.Stalled(); len(lines) != 0 { + t.Fatalf("the failure is the operator's before its author had two hours: %+v", lines) + } + w.later(2*time.Hour + time.Minute) lines := w.h.Stalled() if len(lines) != 1 || lines[0].ID != id || strings.Contains(lines[0].Says, "marked") { t.Fatalf("the failure is not a stalled line: %+v", lines) 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 272f102d..301ed7ed 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -376,6 +376,11 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { d.ReviewedBy, d.ReviewedAt = by, at h.keep(d) } + // So is the line naming who pushed it (ADR 0505 §5). + if by := PushedIn(p.Body); p.Body != "" && by != d.PushedBy { + d.PushedBy = by + h.keep(d) + } // An `after:` line written into the description later is told about too (novox/hq ADR 0419 §2). if after := AfterIn(p.Body); p.Body != "" && !slices.Equal(after, d.After) { d.After = after @@ -453,6 +458,7 @@ func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) { d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid d.Issues, d.After = IssuesIn(p.Title+"\n"+p.Body), AfterIn(p.Body) d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body) + d.PushedBy = PushedIn(p.Body) if p.BaseSHA != "" { d.BaseSHA = p.BaseSHA } @@ -506,6 +512,7 @@ func (h *Holder) PullEdited(p PullEvent) { } before, _ := reviewWords(d) d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body) + d.PushedBy = PushedIn(p.Body) d.After = AfterIn(p.Body) title := p.Title if title == "" { @@ -573,6 +580,7 @@ func (h *Holder) freshAnnouncement(p PullEvent, now time.Time, startedOver strin PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid, After: AfterIn(p.Body), Issues: IssuesIn(p.Title + "\n" + p.Body), Created: now, BaseSHA: p.BaseSHA} d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body) + d.PushedBy = PushedIn(p.Body) why := fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number) if startedOver != "" { why = fmt.Sprintf("%s; %s", why, startedOver) 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 9425bfbe..f025d9a9 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -135,6 +135,9 @@ type Delivery struct { // hold: no Approve is asked until it names the head (novox/hq ADR 0496, issue 1803). ReviewedBy string `json:"reviewed_by,omitempty"` ReviewedAt string `json:"reviewed_at,omitempty"` + // PushedBy is the session that pushed its head, as its pull request's description names it in a line + // `Pushed-by: ` (playbook 07): named with its branch when a new head is needed (novox/hq ADR 0505 §5). + PushedBy string `json:"pushed_by,omitempty"` // FreshWanted is whether a passing verdict that arrived out of date is to have its check asked against the // trunk's head, before the operator is asked (novox/hq ADR 0421 §3, issue 1022). Cleared when it is asked. FreshWanted bool `json:"fresh_wanted,omitempty"` @@ -498,6 +501,31 @@ func ReviewIn(body string) (who, sha string) { return who, sha } +// pushedLine is the line naming the session that pushed a pull request's head: `Pushed-by: ` (ADR 0505 §5). +var pushedLine = regexp.MustCompile(`(?im)^\s*Pushed-by:\s*([A-Za-z0-9_.\-]+(?:@[A-Za-z0-9_.\-]+)?)\s*$`) + +// PushedIn reads it: the last such line, or "" when there is none. +func PushedIn(body string) string { + who := "" + for _, m := range pushedLine.FindAllStringSubmatch(body, -1) { + who = m[1] + } + return who +} + +// authorOf is a delivery's author as the operator reads it: "the author of ", and the session that pushed it +// when its description names one ("who on node": the router refuses an @ in its words). +func authorOf(d *Delivery) string { + who := "the author of " + d.Branch + if d.Branch == "" { + who = "its author" + } + if d.PushedBy != "" { + who += " (" + strings.Replace(d.PushedBy, "@", " on ", 1) + ")" + } + return who +} + // reviewWords is what the delivery's review record says of its head, for the ask: reviewed by whom, or, first, that it // is not reviewed at this head (the ask is held then, ADR 0496). ok is whether it is reviewed at this head. func reviewWords(d *Delivery) (words string, ok bool) { 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 6463e0b3..b68d1017 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -480,10 +480,17 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { Bound: headHeardBound.String(), H2: "none: a later announcement of the pull request's head (the merge tool announces it again, the forge's announcer restarts, or a new push) or an edit of its description; a recheck does not fill it — the operator's", Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)}) case r.Failed != "" && d.State == Rejected: - out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), - Bound: "0s", H2: "none: a new head, or a recheck that passes — the operator's", - Says: fmt.Sprintf("%s#%d passed its checks at %s and is not merged: %s", d.Repository, d.Number, - shortOf(r.Head, 12), r.Failed)}) + // Its cure is its author's new head: listed here at once (`show`), told on the pull request at once, and the + // operator's only once no new head came within the author's bound (novox/hq ADR 0505 §5). + if now.Sub(d.Since) < h.authorBound() { + continue + } + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(d.Since).Round(time.Second).String(), + Bound: h.authorBound().String(), H2: "none: a new head from its author, or a recheck that passes", + Needs: NeedsNewHead, Author: authorOf(d), Branch: d.Branch, + Says: fmt.Sprintf("%s#%d passed its checks at %s and is not merged: %s. It needs a new head from %s, "+ + "and none came for %s", d.Repository, d.Number, shortOf(r.Head, 12), r.Failed, authorOf(d), + now.Sub(d.Since).Round(time.Minute))}) case r.Refused != "": out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), Bound: "0s", H2: "none: a new head, which asks the operator again — the operator's", 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 3545cfcd..b20ecf9a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go @@ -233,8 +233,16 @@ type StalledLine struct { Says string `json:"says"` // Severity is "urgent" for a group that is held or half merged (issue 1275); empty otherwise. Severity string `json:"severity,omitempty"` + // Needs is NeedsNewHead for a delivery whose cure is its author's new head (novox/hq ADR 0505 §5), with Author + // naming that author as the operator reads it and Branch its pull request's branch; empty otherwise. + Needs string `json:"needs,omitempty"` + Author string `json:"author,omitempty"` + Branch string `json:"branch,omitempty"` } +// NeedsNewHead is what a stalled line needs when its cure is its author's new head (ADR 0505 §5). +const NeedsNewHead = "new-head" + // Stalled is the `stalled` verb: what the controller's self-check raises as `delivery..stalled`. func (h *Holder) Stalled() []StalledLine { h.mu.Lock()