diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go
index ded86f62..c035e450 100644
--- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go
+++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go
@@ -404,6 +404,9 @@ type fakeForge struct {
// merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722).
merges, comments, unlabels []string
refuse error
+ // calls are the labels and comments in the order asked (novox/hq issue 913); labelErr fails a Label.
+ calls []string
+ labelErr error
}
func (f *fakeForge) OpenPulls() ([]OpenPull, error) {
@@ -670,6 +673,17 @@ func (f *fakeForge) Comment(owner, repo string, number int, body string) error {
f.mu.Lock()
defer f.mu.Unlock()
f.comments = append(f.comments, fmt.Sprintf("%s/%s#%d: %s", owner, repo, number, body))
+ f.calls = append(f.calls, "comment "+body)
+ return nil
+}
+
+func (f *fakeForge) Label(owner, repo string, number int, name string) error {
+ f.mu.Lock()
+ defer f.mu.Unlock()
+ if f.labelErr != nil {
+ return f.labelErr
+ }
+ f.calls = append(f.calls, fmt.Sprintf("label %s/%s#%d %s", owner, repo, number, name))
return nil
}
diff --git a/modules/mesh-delivery/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go
index cd24e09b..724b6b26 100644
--- a/modules/mesh-delivery/cmd/mesh-delivery/main.go
+++ b/modules/mesh-delivery/cmd/mesh-delivery/main.go
@@ -352,5 +352,19 @@ func tools(h *Holder, l *listening) []stdio.Tool {
"and tried again, given up) and when the last was, walks with no delivery yet, whether its events arrive, when " +
"the controller's walks were last read or why they could not be, and the last refusals.",
Run: func(map[string]any) (any, error) { return h.Status(l.get()), nil }},
+ {Name: seat + "mark-ready",
+ Description: "A reviewer's word that a delivery's head is approved: the mesh's own forge account puts the " +
+ "label mesh/ready on its pull request and then writes \"ready at
\", the order the forge counts. " +
+ "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, issue 913).",
+ Input: map[string]any{"type": "object", "properties": map[string]any{"id": str("the delivery's id (owner/repository@commit): the head reviewed"),
+ "reviewed-by": str("who reviewed it, as who@node"), "why": str("the review's verdict in a line")},
+ "required": []string{"id", "reviewed-by", "why"}},
+ Run: func(a map[string]any) (any, error) {
+ if err := need(a, "id", "reviewed-by", "why"); err != nil {
+ return nil, err
+ }
+ return h.MarkReady(strArg(a, "id"), strArg(a, "reviewed-by"), strArg(a, "why"))
+ }},
}
}
diff --git a/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go
new file mode 100644
index 00000000..703bdd78
--- /dev/null
+++ b/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go
@@ -0,0 +1,143 @@
+package main
+
+import (
+ "errors"
+ "strings"
+ "testing"
+ "time"
+)
+
+// **A reviewer's word marks the pull request ready in the order the forge counts** (novox/hq issue 913): the label,
+// then the comment "ready at ". Written the other way round the mark's head is empty (issue 873).
+
+const fullHead = "3333333333333333333333333333333333333333"
+
+// readyDelivery is a delivery of a pull request whose head is fullHead, its check passed, and the forge's open pull.
+func readyDelivery(t *testing.T, w *world) string {
+ t.Helper()
+ w.h.PullUpdated(pr("novox/app", 7, fullHead, "feat/x", "src/a.go"))
+ w.h.Checked(verdict("novox/app", 7, fullHead, "pass"))
+ w.settleAll()
+ w.forge.open = []OpenPull{{Owner: "novox", Repo: "app", Number: 7, Base: "main", HeadSHA: fullHead}}
+ return IDOf("novox/app", fullHead)
+}
+
+func TestMarkReadyLabelsThenCommentsTheFullHead(t *testing.T) {
+ w := newWorld(t)
+ id := readyDelivery(t, w)
+ answer, err := w.h.MarkReady(id, "fix-913@shanks", "the change is right")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(w.forge.calls) != 2 || w.forge.calls[0] != "label novox/app#7 mesh/ready" {
+ t.Fatalf("the forge was asked %q; wanted the label first", w.forge.calls)
+ }
+ c := strings.TrimPrefix(w.forge.calls[1], "comment ")
+ if first, _, _ := strings.Cut(c, "\n"); first != "ready at "+fullHead || !strings.Contains(c, "\n\nReviewed by fix-913@shanks: the change is right.") ||
+ !strings.Contains(c, "mark-ready") {
+ t.Fatalf("the comment is %q", c)
+ }
+ d := w.delivery(id)
+ if d.Ready == nil || d.Ready.Head != fullHead || d.Ready.By != "fix-913@shanks" || d.Ready.Seen.IsZero() {
+ t.Fatalf("the mark is not kept: %+v", d.Ready)
+ }
+ if !strings.Contains(answer, "marked") {
+ t.Fatalf("answer %q", answer)
+ }
+}
+
+func TestMarkReadyRefusesAMovedHeadAndAsksTheForgeNothing(t *testing.T) {
+ w := newWorld(t)
+ id := readyDelivery(t, w)
+ w.forge.open[0].HeadSHA = pushed
+ _, err := w.h.MarkReady(id, "fix-913@shanks", "ok")
+ if err == nil || !strings.Contains(err.Error(), "the head is now "+pushed) {
+ t.Fatalf("a moved head: %v", err)
+ }
+ if len(w.forge.calls) != 0 {
+ t.Fatalf("the forge was asked %q", w.forge.calls)
+ }
+}
+
+func TestMarkReadyRefusesAnUnknownAndAFinalDelivery(t *testing.T) {
+ w := newWorld(t)
+ id := readyDelivery(t, w)
+ if _, err := w.h.MarkReady("novox/app@nothing", "a@b", "ok"); err == nil || !strings.Contains(err.Error(), "deliveries") {
+ t.Fatalf("unknown id: %v", err)
+ }
+ w.h.mu.Lock()
+ w.h.deliveries[id].State = Stopped
+ w.h.mu.Unlock()
+ if _, err := w.h.MarkReady(id, "a@b", "ok"); err == nil {
+ t.Fatal("a final delivery was marked")
+ }
+ if len(w.forge.calls) != 0 {
+ t.Fatalf("the forge was asked %q", w.forge.calls)
+ }
+}
+
+func TestMarkReadyWritesNoCommentWhenTheLabelFails(t *testing.T) {
+ w := newWorld(t)
+ id := readyDelivery(t, w)
+ w.forge.labelErr = errors.New("the forge refused gitea_api: no such label")
+ _, err := w.h.MarkReady(id, "a@b", "ok")
+ if err == nil || !strings.Contains(err.Error(), "no such label") {
+ t.Fatalf("label failure: %v", err)
+ }
+ if len(w.forge.comments) != 0 {
+ t.Fatalf("commented: %v", w.forge.comments)
+ }
+ if w.delivery(id).Ready != nil {
+ t.Fatal("marked though the label failed")
+ }
+}
+
+func TestClearMarkWithAnEmptyHeadSaysWhatWasWrongOnceItStayedEmpty(t *testing.T) {
+ w := newWorld(t)
+ w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
+ w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")}
+ w.h.SeeOpenPulls()
+ w.settleAll()
+ if len(w.forge.comments) != 0 || len(w.forge.unlabels) != 0 {
+ t.Fatalf("an empty-head mark seen once was cleared: %v", w.forge.comments)
+ }
+ w.later(readyTryEvery + time.Second)
+ w.h.SeeOpenPulls()
+ w.settleAll()
+ if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at\" comment by a listed account") ||
+ !strings.Contains(w.forge.comments[0], "the head is now "+readyHead) ||
+ !strings.Contains(w.forge.comments[0], "mesh-delivery.mark-ready") || strings.Contains(w.forge.comments[0], "pushed after it") {
+ t.Fatalf("comments: %v", w.forge.comments)
+ }
+}
+
+func TestAMarkReadyJustMadeIsNotClearedByAReadThatPredatesItsComment(t *testing.T) {
+ w := newWorld(t)
+ id := readyDelivery(t, w)
+ if _, err := w.h.MarkReady(id, "a@b", "ok"); err != nil {
+ t.Fatal(err)
+ }
+ w.later(readyTryEvery + time.Second) // the label alone was read long enough ago to count as stale...
+ w.forge.open = []OpenPull{markedPull("novox/app", 7, fullHead, "")}
+ w.h.SeeOpenPulls()
+ w.h.mu.Lock()
+ w.h.deliveries[id].Ready.Seen = w.now // ...but the mark was made just now
+ w.h.marks["novox/app#7"].firstEmpty = w.now.Add(-2 * readyTryEvery)
+ w.h.mu.Unlock()
+ before := len(w.forge.comments)
+ w.settleAll()
+ if len(w.forge.unlabels) != 0 || len(w.forge.comments) != before {
+ t.Fatalf("a mark just made was cleared: %v %v", w.forge.unlabels, w.forge.comments)
+ }
+}
+
+func TestClearMarkWithAnOlderHeadKeepsTheOldWording(t *testing.T) {
+ w := newWorld(t)
+ w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go"))
+ w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)}
+ w.h.SeeOpenPulls()
+ w.settleAll()
+ if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "pushed after it") {
+ t.Fatalf("comments: %v", w.forge.comments)
+ }
+}
diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ports.go b/modules/mesh-delivery/cmd/mesh-delivery/ports.go
index 63fb608f..06fa071c 100644
--- a/modules/mesh-delivery/cmd/mesh-delivery/ports.go
+++ b/modules/mesh-delivery/cmd/mesh-delivery/ports.go
@@ -96,6 +96,8 @@ type Forge interface {
Merge(owner, repo string, number int, head string) error
// Comment adds a comment to a pull request.
Comment(owner, repo string, number int, body string) error
+ // Label puts a label on a pull request, by the label's name (novox/hq issue 913).
+ Label(owner, repo string, number int, name string) error
// Unlabel takes a label off a pull request, by the label's id.
Unlabel(owner, repo string, number, label int) error
}
@@ -407,6 +409,47 @@ func (f toolForge) Comment(owner, repo string, number int, body string) error {
return f.tool("gitea_add_comment", map[string]any{"owner": owner, "repo": repo, "number": number, "body": body})
}
+// Label puts a label on a pull request. The forge's add-labels call takes label ids on every version (names only on
+// the newer), so the id is read from the repository's labels by name first; a repository without the label is refused,
+// not given one.
+func (f toolForge) Label(owner, repo string, number int, name string) error {
+ id := 0
+ for page := 1; page <= 20 && id == 0; page++ {
+ raw, err := f.ask("gitea.gitea_api", map[string]any{"method": "GET",
+ "path": fmt.Sprintf("/repos/%s/%s/labels?limit=50&page=%d", owner, repo, page)})
+ if err != nil {
+ return err
+ }
+ answer, output, ok := answerOf(raw)
+ if !ok {
+ return fmt.Errorf("the forge refused gitea_api: %s", lastLine(output))
+ }
+ var got struct {
+ Result []struct {
+ ID int `json:"id"`
+ Name string `json:"name"`
+ } `json:"result"`
+ }
+ if err := json.Unmarshal(answer, &got); err != nil {
+ return fmt.Errorf("the forge's labels are not readable: %s", clip(string(answer), 200))
+ }
+ for _, l := range got.Result {
+ if l.Name == name {
+ id = l.ID
+ }
+ }
+ if len(got.Result) < 50 {
+ break
+ }
+ }
+ if id == 0 {
+ return fmt.Errorf("%s/%s has no label %s", owner, repo, name)
+ }
+ return f.tool("gitea_api", map[string]any{"method": "POST",
+ "path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels", owner, repo, number),
+ "body": map[string]any{"labels": []int{id}}})
+}
+
func (f toolForge) Unlabel(owner, repo string, number, label int) error {
return f.tool("gitea_api", map[string]any{"method": "DELETE",
"path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels/%d", owner, repo, number, label)})
diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go
index f5488f25..f6f98672 100644
--- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go
+++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go
@@ -88,6 +88,9 @@ type marked struct {
pull OpenPull
// cleared is when the mark was cleared, the label taken off: not cleared again within readyTryEvery.
cleared time.Time
+ // firstEmpty is when a mark with no head (a label no "ready at" comment follows yet) was first read so: it is a
+ // mark not yet complete, and is cleared only once it has stayed so for readyTryEvery (novox/hq issue 873).
+ firstEmpty time.Time
}
// pullKey is a pull request as a key: owner/repo#number.
@@ -109,6 +112,13 @@ func (h *Holder) seeMarks(open []OpenPull) {
was.pull.HeadSHA == p.HeadSHA {
m.cleared = was.cleared
}
+ if p.Ready.Head == "" {
+ m.firstEmpty = h.Now()
+ if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == "" &&
+ was.pull.HeadSHA == p.HeadSHA && !was.firstEmpty.IsZero() {
+ m.firstEmpty = was.firstEmpty
+ }
+ }
seen[k] = m
}
h.marks = seen
@@ -129,6 +139,13 @@ func (h *Holder) readyAsks(now time.Time) []func() {
p, mark := m.pull, *m.pull.Ready
repository := strings.ToLower(p.Owner + "/" + p.Repo)
if !sameFull(mark.Head, p.HeadSHA) {
+ if mark.Head == "" && now.Sub(m.firstEmpty) < readyTryEvery {
+ continue // the label first, its comment not yet: a mark not complete, not an old one
+ }
+ if d := h.deliveries[IDOf(repository, p.HeadSHA)]; d != nil && d.Ready != nil &&
+ sameFull(d.Ready.Head, p.HeadSHA) && now.Sub(d.Ready.Seen) < readyTryEvery {
+ continue // marked through mark-ready a moment ago: this read may predate its comment
+ }
if !m.cleared.IsZero() && now.Sub(m.cleared) < readyTryEvery {
continue
}
@@ -270,22 +287,41 @@ func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() {
return func() {
err := h.Forge.Unlabel(p.Owner, p.Repo, p.Number, mark.LabelID)
if err == nil {
- err = h.Forge.Comment(p.Owner, p.Repo, p.Number, fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+
- "the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA))
+ err = h.Forge.Comment(p.Owner, p.Repo, p.Number, clearedSaying(mark, p))
}
- if err != nil {
- h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v",
- k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err)
- return
+ if mark.Head == "" {
+ if err != nil {
+ h.Logf("[mesh-delivery] %s's ready label has no \"ready at \" comment of a listed account after it, and could not be taken off yet: %v", k, err)
+ return
+ }
+ h.Logf("[mesh-delivery] %s's ready label taken off: no \"ready at \" comment of a listed account was written after it; mark-ready writes both in order", k)
+ } else {
+ if err != nil {
+ h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v",
+ k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err)
+ return
+ }
+ h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12),
+ shortOf(p.HeadSHA, 12))
}
- h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12),
- shortOf(p.HeadSHA, 12))
h.mu.Lock()
delete(h.marks, k)
h.mu.Unlock()
}
}
+// clearedSaying is what a cleared mark says on its pull request. A mark with no head is a label no "ready at "
+// comment of a listed account followed (novox/hq issue 873); one with a head names an older head than the pull's.
+func clearedSaying(mark ReadyMark, p OpenPull) string {
+ if mark.Head == "" {
+ return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at\" comment by a listed account " +
+ "follows both the label's last addition and the last push; the head is now " + p.HeadSHA + ". Review it and " +
+ "mark that head with `mesh-delivery.mark-ready`, which writes label and comment in the order the forge counts."
+ }
+ return fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+
+ "the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA)
+}
+
// sayOn comments on a delivery's pull request.
func (h *Holder) sayOn(d *Delivery, body string) func() {
owner, repo, number, id := d.Owner(), d.Repo(), d.Number, d.ID
diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go
index e44c2236..f16530a4 100644
--- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go
+++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go
@@ -331,6 +331,65 @@ func (h *Holder) Recheck(id, why, by string) (string, error) {
return fmt.Sprintf("%s is proposed again, its merge check pending; its check was asked (%s)", id, asked), nil
}
+// MarkReady is a reviewer's word that a delivery's head is approved (novox/hq issue 913): the label mesh/ready on its
+// pull request, and only after that call succeeded the comment "ready at ", the order the forge counts (a comment
+// written before the label does not count, issue 873). Refused for an unknown, final or merged delivery, and when the
+// pull request's head is no longer the delivery's commit.
+func (h *Holder) MarkReady(id, reviewedBy, why string) (string, error) {
+ h.mu.Lock()
+ d := h.deliveries[id]
+ if d == nil {
+ h.mu.Unlock()
+ return "", fmt.Errorf("no delivery %q; `deliveries` lists them", id)
+ }
+ if d.State.Final() || d.MergedAs != "" {
+ h.mu.Unlock()
+ return "", fmt.Errorf("%s is %s: a delivery past its merge or final is not marked ready", id, d.State)
+ }
+ if d.Number == 0 {
+ h.mu.Unlock()
+ return "", fmt.Errorf("%s is not of a pull request: only a pull request is marked ready", id)
+ }
+ owner, repo, number, commit := d.Owner(), d.Repo(), d.Number, d.Commit
+ h.mu.Unlock()
+
+ open, err := h.Forge.OpenPulls()
+ if err != nil {
+ return "", fmt.Errorf("%s is not marked: the forge's open pull requests could not be read: %v", id, err)
+ }
+ head := ""
+ for _, p := range open {
+ if strings.EqualFold(p.Owner, owner) && strings.EqualFold(p.Repo, repo) && p.Number == number {
+ head = p.HeadSHA
+ }
+ }
+ if head == "" {
+ return "", fmt.Errorf("%s#%d is not an open pull request on the forge", owner+"/"+repo, number)
+ }
+ if !sameCommit(head, commit) {
+ return "", fmt.Errorf("the head is now %s; review and mark that one", head)
+ }
+ if err := h.Forge.Label(owner, repo, number, ReadyLabel); err != nil {
+ return "", fmt.Errorf("%s is not marked: the label %s could not be put on: %v", id, ReadyLabel, err)
+ }
+ body := fmt.Sprintf("ready at %s\n\nReviewed by %s: %s. Marked through mesh-delivery.mark-ready (novox/hq issue 913).",
+ head, reviewedBy, strings.TrimRight(strings.TrimSpace(why), "."))
+ if err := h.Forge.Comment(owner, repo, number, body); err != nil {
+ return "", fmt.Errorf("%s has the label %s but its comment \"ready at\" could not be written: %v; mark it again", id, ReadyLabel, err)
+ }
+ h.mu.Lock()
+ defer h.mu.Unlock()
+ now := h.Now()
+ d.Ready = &ReadyRun{Head: head, By: reviewedBy, MarkedAt: now.UTC().Format(time.RFC3339), Seen: now}
+ h.Logf("[mesh-delivery] %s is marked ready at %s by %s through mark-ready: %s", id, shortOf(head, 12), reviewedBy, why)
+ h.keep(d)
+ waits := h.readyWaits(d)
+ if waits == "" {
+ return fmt.Sprintf("%s is marked ready at %s; its merge is asked once the next tick reads the mark", id, shortOf(head, 12)), nil
+ }
+ return fmt.Sprintf("%s is marked ready at %s; its merge waits: %s", id, shortOf(head, 12), waits), nil
+}
+
// checkingAgain puts a head's merge check statuses — those a branch's protection may require — back to
// pending, so the forge refuses the merge until the fresh verdict sets them. The gate always, since every
// verdict sets it; the repository's own check only when the head holds one, since a verdict that says none
diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json
index 4c486145..b32b6320 100644
--- a/modules/mesh-delivery/module.json
+++ b/modules/mesh-delivery/module.json
@@ -19,7 +19,8 @@
"release",
"stop",
"close",
- "retire-history"
+ "retire-history",
+ "mark-ready"
]
}
],