diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval.go b/modules/mesh-delivery/cmd/mesh-delivery/approval.go index 41b46e18..4a0e91f2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/approval.go @@ -442,6 +442,7 @@ func (h *Holder) decided(id, state string, w *asks.Warrant) []func() { for _, d := range covered { r := d.Ready r.Warrant = w.Says() + h.mergeAgainNow(d) // the operator chose: the merge is asked at once switch option.ID { case OptionApprove: r.Approved = &now diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events.go b/modules/mesh-delivery/cmd/mesh-delivery/events.go new file mode 100644 index 00000000..8143dc20 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/events.go @@ -0,0 +1,114 @@ +package main + +import ( + "regexp" + "strconv" + "strings" + + "git.novox.be/novox/mesh-sdk/go/asks" +) + +// Events that start a step (novox/hq ADR 0406 §1, issue 975). +// +// This owner acts on events: a ready mark, a verdict, an Approve, a group's turn. The forge's label and comment events +// say that a pull request's ready mark may have changed; they do not say whether it counts. Which accounts' "ready at" +// comments count is the forge module's rule (its trusted setting ready-authors, the gitea module's readyMarkOf), so a +// label added or a "ready at" comment written makes this owner read the forge's open pull requests once, and the mark +// it reads there is the mark (seeMarks). The label taken off needs no read: it only clears. + +// ForgePullEvent is the forge module's `forge.pull-request.labeled`, `.unlabeled` and `.commented` +// (modules/gitea/webhook.ts): the pull request, and the label added or taken off, or the comment written. +type ForgePullEvent struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + HeadSHA string `json:"head_sha,omitempty"` + Base string `json:"base,omitempty"` + Labels []string `json:"labels,omitempty"` + Label string `json:"label,omitempty"` + Author string `json:"author,omitempty"` + Comment string `json:"comment,omitempty"` + CommentID int `json:"comment_id,omitempty"` +} + +// ForgeStatusEvent is the forge module's `forge.status`: a commit's status was set. +type ForgeStatusEvent struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + SHA string `json:"sha"` + Context string `json:"context"` + State string `json:"state"` +} + +// readyAtComment is a comment that names the head a pull request is marked ready at, as the forge module reads it +// (modules/gitea/client.ts READY_AT): "ready at " on a line of its own. +var readyAtComment = regexp.MustCompile(`(?im)^\s*ready at ([0-9a-f]{40}|[0-9a-f]{64})\b`) + +func eventPullKey(e ForgePullEvent) string { + return strings.ToLower(e.Owner+"/"+e.Repo) + "#" + strconv.Itoa(e.Number) +} + +// wantForge says the forge's open pull requests are to be read at the next look: a ready mark may have been made. +func (h *Holder) wantForge() { + h.mu.Lock() + h.forgeWanted = true + h.mu.Unlock() +} + +// takeForgeWanted answers whether the forge's open pull requests are to be read, and takes the want. +func (h *Holder) takeForgeWanted() bool { + h.mu.Lock() + defer h.mu.Unlock() + wanted := h.forgeWanted + h.forgeWanted = false + return wanted +} + +// PullLabeled takes a label added to a pull request: the ready label asks for the mark to be read. +func (h *Holder) PullLabeled(e ForgePullEvent) { + if e.Label == ReadyLabel { + h.wantForge() + } +} + +// PullUnlabeled takes a label taken off a pull request: the ready label clears its mark, with no read. +func (h *Holder) PullUnlabeled(e ForgePullEvent) { + if e.Label != ReadyLabel { + return + } + h.mu.Lock() + defer h.mu.Unlock() + if _, marked := h.marks[eventPullKey(e)]; marked { + delete(h.marks, eventPullKey(e)) + h.Logf("[mesh-delivery] %s's ready label was taken off: its mark is cleared", eventPullKey(e)) + } +} + +// PullCommented takes a comment on a pull request: one that names a head to be ready at asks for the mark to be read. +func (h *Holder) PullCommented(e ForgePullEvent) { + if readyAtComment.MatchString(e.Comment) { + h.wantForge() + } +} + +// React takes every step the facts allow, after an event that can change what they are: the forge's open pull requests +// read when a ready mark may have been made, then the asks of a look (look). Never concurrent with itself. +func (h *Holder) React() { + h.reactMu.Lock() + defer h.reactMu.Unlock() + if h.takeForgeWanted() { + h.SeeOpenPulls() + } + h.look() +} + +// AskDecided takes the router's `decided` event for an ask this owner made: its warrant is applied to the deliveries +// the ask covers, and what is said on their pull requests is said. +func (h *Holder) AskDecided(id, state string, w *asks.Warrant) { + h.mu.Lock() + said := h.decided(id, state, w) + h.mu.Unlock() + for _, s := range said { + s() + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events_test.go b/modules/mesh-delivery/cmd/mesh-delivery/events_test.go new file mode 100644 index 00000000..f270386c --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/events_test.go @@ -0,0 +1,160 @@ +package main + +import ( + "encoding/json" + "testing" + + stdio "git.novox.be/novox/mesh-sdk/go" +) + +// **A ready mark, a verdict and an Approve are acted on as events** (novox/hq ADR 0406 §1, issue 975): the merge +// is asked within the event that makes it possible, never at the next tick or the next minute. + +const ( + labeledEvent = "gitea.forge.pull-request.labeled" + unlabeledEvent = "gitea.forge.pull-request.unlabeled" + commentedEvent = "gitea.forge.pull-request.commented" +) + +// hear is the runtime handing this owner one event, as handle takes it. +func hear(t *testing.T, w *world, key string, body any) { + t.Helper() + raw, err := json.Marshal(body) + if err != nil { + t.Fatal(err) + } + if err := handle(w.h, stdio.Envelope{Key: key, Body: raw}); err != nil { + t.Fatal(err) + } +} + +func labeled(number int, label string) ForgePullEvent { + return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, HeadSHA: readyHead, Base: "main", Label: label, + Labels: []string{label}} +} + +func commentedReady(number int, head string) ForgePullEvent { + return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, Author: "reviewer", CommentID: 5, + Comment: "ready at " + head + "\n\napproved: the tests are the change's"} +} + +// greenAnnounced is a pull request into main, announced and judged green, the operator not answering by himself. +func greenAnnounced(t *testing.T) *world { + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + return w +} + +func TestALabelAndAReadyAtCommentAskTheOperatorAtOnceWithNoTick(t *testing.T) { + w := greenAnnounced(t) + // The label first: the forge reads it without a comment naming a head yet. Nothing is asked. + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")} + hear(t, w, labeledEvent, labeled(7, ReadyLabel)) + if w.op.count() != 0 || len(w.forge.merges) != 0 { + t.Fatalf("a label with no head named was acted on: %d asks, merges %v", w.op.count(), w.forge.merges) + } + // The comment naming the head: the forge now reads the mark whole, and the ask is made within this event. + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + hear(t, w, commentedEvent, commentedReady(7, readyHead)) + if w.op.count() != 1 { + t.Fatalf("the comment that completes the mark asked the operator %d times, wanted once, with no tick", w.op.count()) + } + if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready == nil || d.Ready.By != "reviewer" || d.Ready.AskID == "" { + t.Fatalf("the mark is not kept on the delivery: %+v", d.Ready) + } +} + +func TestACommentThatIsNotAReadyMarkReadsNothingFromTheForge(t *testing.T) { + w := greenAnnounced(t) + before := w.forge.openCalls + c := commentedReady(7, readyHead) + c.Comment = "looks fine to me, though I would not say ready at all" + hear(t, w, commentedEvent, c) + hear(t, w, labeledEvent, labeled(7, "kind/bug")) + if w.forge.openCalls != before { + t.Fatalf("the forge's open pull requests were read %d times for events that are no ready mark", w.forge.openCalls-before) + } +} + +func TestTheReadyLabelTakenOffClearsTheMarkAtOnce(t *testing.T) { + w := greenAnnounced(t) + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + hear(t, w, labeledEvent, labeled(7, ReadyLabel)) + w.h.mu.Lock() + marks := len(w.h.marks) + w.h.mu.Unlock() + if marks != 1 { + t.Fatalf("the label did not make a mark: %d", marks) + } + asked := w.op.count() + // The forge no longer shows the label; the event alone says so, and the forge is not read for it. + reads := w.forge.openCalls + w.forge.open = nil + hear(t, w, unlabeledEvent, labeled(7, ReadyLabel)) + w.h.mu.Lock() + marks = len(w.h.marks) + w.h.mu.Unlock() + if marks != 0 || w.forge.openCalls != reads || w.op.count() != asked { + t.Fatalf("the mark was not cleared by the event alone: marks %d, reads %d, asks %d", marks, w.forge.openCalls-reads, w.op.count()-asked) + } +} + +// approve is the operator's Approve of the one open ask, heard as the router's `decided` event. +func approve(w *world) { + open := w.op.open() + w.op.answer(open[0], OptionApprove) + state, warrant, _ := w.op.Where(open[0]) + w.h.AskDecided(open[0], state, warrant) + w.h.React() +} + +func TestAnApproveHeardAsAnEventAsksTheMergeAtOnce(t *testing.T) { + w := greenAnnounced(t) + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + hear(t, w, commentedEvent, commentedReady(7, readyHead)) + open := w.op.open() + if len(open) != 1 || len(w.forge.merges) != 0 { + t.Fatalf("asks %v, merges %v", open, w.forge.merges) + } + approve(w) + if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/app#7@"+readyHead { + t.Fatalf("the Approve, heard as an event, merged %v; wanted once at the marked head", w.forge.merges) + } +} + +func TestAPassingVerdictAsksTheMergeAtOnceEvenJustAfterARefusal(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", readyHead) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + // Another merge moves the trunk: the verdict is out of date and the merge tool refuses, asking the check again. + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) + w.forge.refuse = errAskedAgain + w.op.auto = false + w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + hear(t, w, commentedEvent, commentedReady(7, readyHead)) + approve(w) + if len(w.forge.merges) != 1 { + t.Fatalf("the merge tool was asked %v; wanted once to judge it again", w.forge.merges) + } + // The verdict against the trunk's head passes: merged within the verdict's own event, no minute waited. + w.forge.refuse = nil + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + raw, _ := json.Marshal(judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) + if err := handle(w.h, stdio.Envelope{Key: ControllerSeat + ".checked", Body: raw}); err != nil { + t.Fatal(err) + } + if len(w.forge.merges) != 2 || w.delivery(id).Ready.Merged == nil { + t.Fatalf("the passing verdict did not ask the merge at once: %v %+v", w.forge.merges, w.delivery(id).Ready) + } +} + +var errAskedAgain = &askedAgainError{} + +type askedAgainError struct{} + +func (*askedAgainError) Error() string { + return "the forge refused gitea_merge_pull_request: The check of head merged into trunk was asked" +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index effcd900..565d29e4 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -399,8 +399,9 @@ type fakeForge struct { containsDown bool containsAsks []string // open is what OpenPulls answers; openDown a forge that cannot say. - open []OpenPull - openDown bool + open []OpenPull + openDown bool + openCalls int // merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722). merges, comments, unlabels []string refuse error @@ -412,6 +413,7 @@ type fakeForge struct { func (f *fakeForge) OpenPulls() ([]OpenPull, error) { f.mu.Lock() defer f.mu.Unlock() + f.openCalls++ if f.openDown { return nil, errors.New("the forge is away") } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 42626b6a..05de5284 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -84,6 +84,10 @@ type Holder struct { tried owedTries // marks are the open pull requests read with a ready mark, by owner/repo#number (novox/hq issue 722, ready.go). marks map[string]*marked + // forgeWanted is whether the forge's open pull requests are to be read at the next React: a ready mark may have been + // made (events.go). reactMu keeps one React at a time. + forgeWanted bool + reactMu sync.Mutex } // owedTries is how many owed effects were tried since this owner started, and when the last was: three numbers @@ -509,6 +513,7 @@ func (h *Holder) PullMerged(p PullEvent) { } h.mu.Lock() defer h.mu.Unlock() + delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a merged pull request keeps no mark var d *Delivery if p.HeadSHA != "" { d = h.deliveries[IDOf(p.repository(), p.HeadSHA)] @@ -667,6 +672,7 @@ func verdictOfState(s string) string { // go of it within the week anyway. func (h *Holder) PullClosed(p PullEvent) { h.mu.Lock() + delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a closed pull request keeps no mark for _, d := range h.deliveries { if d.Repository == p.repository() && d.Number == p.Number && !d.State.Final() && d.MergedAs == "" { d.ClosedUnmerged = true @@ -736,6 +742,9 @@ func (h *Holder) Checked(c CheckedEvent) { return } g.Check.Verdict, g.Check.Summary = c.Verdict, c.Summary + for _, m := range h.membersOf(g) { + h.mergeAgainNow(m) // the group's composed check answered: its members' merges are asked at once + } // Each member's head and the trunk commit it was merged into (novox/hq ADR 0323 §6). for _, m := range c.Members { for id := range g.Check.Commits { @@ -796,6 +805,7 @@ func (h *Holder) Checked(c CheckedEvent) { v.Gate = "error" } d.Check, d.AskedAgain = v, nil + h.mergeAgainNow(d) // a verdict taken: the merge is asked at once, not a minute after a refusal if c.Plan != nil { d.Plan = c.Plan } @@ -815,6 +825,11 @@ func (h *Holder) WalkMoved(w Walk) { } func (h *Holder) walkMoved(w Walk) { + for _, d := range h.deliveries { + if d.Group != "" { + h.mergeAgainNow(d) // a walk moved: a group member's turn to merge may have come, asked at once + } + } if w.Release != nil { h.creditBacklog(w) return @@ -1055,9 +1070,12 @@ func (h *Holder) Reconcile() error { // --- the tick: groups, turns, adoption -------------------------------------------------------------------- -// Tick takes every step the facts allow that needs another's word: a group's order and composed check, a +// Tick is React, for the tick that still runs beside the events. +func (h *Holder) Tick() { h.React() } + +// look takes every step the facts allow that needs another's word: a group's order and composed check, a // delivery's turn, a walk with no delivery made one. The asking happens outside the lock. -func (h *Holder) Tick() { +func (h *Holder) look() { for _, ask := range h.plan() { ask() } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go index 52fa7fa0..7a246e9d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/main.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/main.go @@ -31,7 +31,9 @@ func (l *listening) set(s string) { l.mu.Lock(); l.now = s; l.mu.Unlock() } func (l *listening) get() string { l.mu.Lock(); defer l.mu.Unlock(); return l.now } // The events this owner follows: the forge's pull requests, and the controller's verdicts and walks. +// The forge's label and comment events say a ready mark changed (events.go). var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", + "gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented", ControllerSeat + ".checked", ControllerSeat + ".plan-moved"} func main() { @@ -104,9 +106,15 @@ func run(h *Holder, l *listening) { logf("[mesh-delivery] listening: %s", strings.Join(followed, ", ")) } -// handle takes one event. An event that cannot be read is said and acknowledged: read again it would fail -// again, and the controller's walks are read back by comparison anyway. +// handle takes one event, then takes every step the event makes possible (React). An event that cannot be read is +// said and acknowledged: read again it would fail again, and the controller's walks are read back by comparison anyway. func handle(h *Holder, e stdio.Envelope) error { + taken := take(h, e) + h.React() + return taken +} + +func take(h *Holder, e stdio.Envelope) error { switch e.Key { case "gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed": var p PullEvent @@ -122,6 +130,20 @@ func handle(h *Holder, e stdio.Envelope) error { default: h.PullClosed(p) } + case "gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented": + var p ForgePullEvent + if err := json.Unmarshal(e.Body, &p); err != nil { + h.Refuse(fmt.Errorf("%s is not readable: %v", e.Key, err)) + return nil + } + switch e.Key { + case "gitea.forge.pull-request.labeled": + h.PullLabeled(p) + case "gitea.forge.pull-request.unlabeled": + h.PullUnlabeled(p) + default: + h.PullCommented(p) + } case ControllerSeat + ".checked": var c CheckedEvent if err := json.Unmarshal(e.Body, &c); err != nil { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 9ac5f36f..0c550f49 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -65,6 +65,8 @@ type ReadyRun struct { Tries int `json:"tries,omitempty"` LastTry time.Time `json:"last_try"` Answer string `json:"answer,omitempty"` + // merging is whether a merge ask is on its way, outside the lock: not asked twice meanwhile. Not kept. + merging bool // RefusedSince is when the merge tool began refusing it without a break. RefusedSince *time.Time `json:"refused_since,omitempty"` // Merged is when the merge tool merged it. @@ -255,11 +257,11 @@ func (h *Holder) readyAsks(now time.Time) []func() { if approvalAsk != nil { asks = append(asks, approvalAsk) } - if waits != "" || (!r.LastTry.IsZero() && now.Sub(r.LastTry) < readyTryEvery) { + if waits != "" || r.merging || (!r.LastTry.IsZero() && now.Sub(r.LastTry) < readyTryEvery) { continue } r.Tries++ - r.LastTry = now + r.LastTry, r.merging = now, true h.keep(d) asks = append(asks, h.askMerge(d.ID, p, mark.Head)) } @@ -268,6 +270,14 @@ func (h *Holder) readyAsks(now time.Time) []func() { return asks } +// mergeAgainNow lets a marked delivery's merge be asked at once: something that decides whether it may merge happened (a +// verdict, an Approve, a group's turn), so the wait after a refusal does not hold it. Under the lock. +func (h *Holder) mergeAgainNow(d *Delivery) { + if d != nil && d.Ready != nil && d.Ready.Merged == nil { + d.Ready.LastTry = time.Time{} + } +} + // readyWaits is what a marked delivery's merge waits on; empty when it is asked now. A ready delivery is asked; so is // a proposed one whose verdict went out of date and whose check nobody asked again — the merge tool asks it. func (h *Holder) readyWaits(d *Delivery) string { @@ -325,6 +335,7 @@ func (h *Holder) askMerge(id string, p OpenPull, head string) func() { return } r, now := d.Ready, h.Now() + r.merging = false if err == nil { r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil h.Logf("[mesh-delivery] %s merged on its ready mark at %s, its check passed against its trunk's head", id, diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json index f951f918..6ff9c526 100644 --- a/modules/mesh-delivery/module.json +++ b/modules/mesh-delivery/module.json @@ -28,6 +28,9 @@ "gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", + "gitea.forge.pull-request.labeled", + "gitea.forge.pull-request.unlabeled", + "gitea.forge.pull-request.commented", "mesh-controller.checked", "mesh-controller.plan-moved" ],