diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bridge_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bridge_test.go index fb16da09..1749fe5a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bridge_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bridge_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "sort" "strconv" "strings" @@ -134,3 +135,67 @@ func (s *fakeSource) Move(update func(prev *sdk.Source) (sdk.Source, bool)) erro func sortStrings(s []string) { sort.Strings(s) } func itoa(n int) string { return strconv.Itoa(n) } + +// fakeOwed is the gate's bucket owed with the forge bridge behind it: every act written is performed on the fake forge, as the +// bridge performs it (mesh-delivery modules/mesh-delivery-forge-bridge internal/bridge acts.go OwedAct), once for each +// revision. With down set the bucket cannot be written (the bus is away); the forge's own refusals are the bridge's to say, +// not the gate's to retry. +type fakeOwed struct { + mu sync.Mutex + forge *fakeForge + down bool + // keys are the latest value of each key written, and writes every revision in order. + keys map[string]sdk.Owed + writes []string +} + +func newFakeOwed(f *fakeForge) *fakeOwed { return &fakeOwed{forge: f, keys: map[string]sdk.Owed{}} } + +func (o *fakeOwed) Put(key string, v sdk.Owed) error { + if _, err := sdk.EncodeOwed(v); err != nil { + return err + } + repository, number, head, act, err := sdk.ParseOwedKey(key) + if err != nil { + return err + } + o.mu.Lock() + if o.down { + defer o.mu.Unlock() + return errors.New("the bus is away") + } + if kept, ok := o.keys[key]; ok && sdk.SameOwed(kept, v) { + o.mu.Unlock() + return nil + } + o.keys[key] = v + o.writes = append(o.writes, key) + o.mu.Unlock() + owner, repo, _ := strings.Cut(repository, "/") + f := o.forge + switch v.Act { + case sdk.OwedNote: + _ = f.Note(owner, repo, head, v.Text) + case sdk.OwedView: + _ = f.View(owner, repo, number, v.Text) + case sdk.OwedStatus: + _ = f.Status(owner, repo, head, v.Context, v.State, v.Text, v.Target) + case sdk.OwedComment: + _ = f.Comment(owner, repo, number, v.Text) + } + _ = act + return nil +} + +// acts are the keys written whose act is of a kind, in order. +func (o *fakeOwed) acts(kind string) []string { + o.mu.Lock() + defer o.mu.Unlock() + var out []string + for _, k := range o.writes { + if _, _, _, act, err := sdk.ParseOwedKey(k); err == nil && actKind(act) == kind { + out = append(out, k) + } + } + return out +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go index e53ac606..33edebaf 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go @@ -543,6 +543,16 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB return err == nil && json.Unmarshal(e.Value(), &k) == nil && k == PullKey{Head: head, Trunk: "t1t1t1t1t1t1", Verdict: "pass"} }) + // What the transitions owe the forge is written as owed keys; the gate calls no tool of the forge that writes (issue 1349). + for _, tool := range []string{"gitea.gitea_commit_status", "gitea.gitea_delivery_view", "gitea.gitea_note_append", "gitea.gitea_add_comment"} { + if n := len(rt.asksOf(tool)); n != 0 { + t.Fatalf("the gate asked %s %d times: the bridge writes to the forge", tool, n) + } + } + rt.until("the owed keys to be written", 5*time.Second, func() bool { + keys, err := b.kvs["owed"].Keys(ctx) + return err == nil && len(keys) > 0 + }) // The operator approves; the teller holds the warrant and writes the answer: the merge is handed to the bridge within // seconds, with the Approve's heads, and the gate asks the forge nothing. approved := b.answer(t, key, OptionApprove) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go index 525a612d..b1e7a92f 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go @@ -3,7 +3,6 @@ package main import ( "encoding/json" "fmt" - "regexp" "sort" "strings" "time" @@ -243,8 +242,9 @@ func (h *Holder) Flush() { group bool e Effect body string - repo [2]string + repo string num int + head string url string } h.mu.Lock() @@ -274,7 +274,7 @@ func (h *Holder) Flush() { if e.Next != nil && now.Before(*e.Next) { continue } - j := job{id: id, e: e, repo: [2]string{d.Owner(), d.Repo()}, num: d.Number, url: d.HTMLURL} + j := job{id: id, e: e, repo: d.Repository, num: d.Number, head: d.Commit, url: d.HTMLURL} if e.Kind == EffectView { j.body = ViewBody(d, h.groups[d.Group]) } @@ -295,15 +295,11 @@ func (h *Holder) Flush() { h.mu.Unlock() for _, j := range jobs { - err := h.do(j.e, j.repo, j.num, j.body, j.url, j.group) + err := h.do(j.e, j.repo, j.num, j.head, j.body, j.url, j.group) h.mu.Lock() - // Why it is given up, when it is: the forge's word that what it is for does not exist is final, and - // so is a failure tried retryLimit times, the last ones retryCap apart. + // Why it is given up, when it is: a failure of the bus tried retryLimit times, the last ones retryCap apart. dropped := "" - switch { - case err != nil && forgeKinds[j.e.Kind] && doesNotExist(err): - dropped = "the forge answers that what it is for does not exist" - case err != nil && j.e.Tries+1 >= retryLimit: + if err != nil && j.e.Tries+1 >= retryLimit { dropped = fmt.Sprintf("tried %d times since %s", j.e.Tries+1, j.e.Since.UTC().Format(time.RFC3339)) } owed := func(list []Effect) []Effect { @@ -314,7 +310,9 @@ func (h *Holder) Flush() { return append(list[:i:i], list[i+1:]...) } list[i].Tries++ - list[i].Last = err.Error() + if o.Kind != EffectStatus || !j.group { + list[i].Last = err.Error() // a group's status names its member in Last, which stays + } next := h.Now().Add(retryAfter(list[i].Tries)) list[i].Next = &next return list @@ -401,17 +399,8 @@ func retryAfter(tries int) time.Duration { return min(wait, retryCap) } -// forgeKinds are the effects done by the forge's holder. -var forgeKinds = map[string]bool{EffectNote: true, EffectView: true, EffectStatus: true} - -// notThere is the forge's own answer that what was asked of does not exist: a pull request whose number the -// forge no longer holds, a commit it does not know. Tried again it answers the same, for ever. -var notThere = regexp.MustCompile(`(?i)Gitea API \S+: (404\b|\d{3}\b.*\bdoes not exist\b)`) - -func doesNotExist(err error) bool { return err != nil && notThere.MatchString(err.Error()) } - -// do is one owed effect. -func (h *Holder) do(e Effect, repo [2]string, number int, body, url string, group bool) error { +// do is one owed effect: said on the bus, or written as the key the forge bridge performs. +func (h *Holder) do(e Effect, repository string, number int, head, body, url string, group bool) error { switch e.Kind { case EffectEmit: var payload any @@ -426,28 +415,20 @@ func (h *Holder) do(e Effect, repo [2]string, number int, body, url string, grou event = e.Event } return h.Emit(event, payload) - case EffectNote: - if h.Forge == nil { - return fmt.Errorf("no forge") - } - return h.Forge.Note(repo[0], repo[1], e.Commit, e.Line) case EffectView: - if h.Forge == nil || number == 0 { + if number == 0 { return nil } - return h.Forge.View(repo[0], repo[1], number, body) - case EffectStatus: - if h.Forge == nil { - return fmt.Errorf("no forge") - } - if group { - // A group's status names its member's repository and page in Last. - where, page, _ := strings.Cut(e.Last, " ") - ownerRepo, _, _ := strings.Cut(where, "#") - owner, name, _ := strings.Cut(ownerRepo, "/") - return h.Forge.Status(owner, name, e.Commit, e.Context, e.State, e.Line, page) - } - return h.Forge.Status(repo[0], repo[1], e.Commit, e.Context, e.State, e.Line, url) } - return nil + page := url + if group && e.Kind == EffectStatus { + // A group's status names its member's repository and page in Last. + repository, number, page = memberOfLast(e.Last) + head = e.Commit + } + act, value, num, hd := owedOf(e, repository, number, head, body, page) + if act == "" { + return nil + } + return h.owe(repository, num, hd, act, value) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go index 3360b384..61de32c0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go @@ -599,6 +599,7 @@ type world struct { op *fakeOperator merges *fakeMerges source *fakeSource + owed *fakeOwed // asked is the key of the ask a test answered. asked string now time.Time @@ -631,6 +632,8 @@ func newWorldOver(t *testing.T, store *memStore, ctl *fakeController, forge *fak w.merges, w.source = newFakeMerges(forge), &fakeSource{} w.h.Merges, w.h.Source = w.merges, w.source w.merges.h = w.h + w.owed = newFakeOwed(forge) + w.h.Owed = w.owed w.h.init() if err := w.h.Load(); err != nil { t.Fatal(err) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/history_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/history_test.go index 382964e4..9d8feccd 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/history_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/history_test.go @@ -2,6 +2,7 @@ package main import ( "errors" + sdk "git.novox.be/novox/mesh-sdk/go/delivery" "strings" "testing" "time" @@ -177,41 +178,31 @@ func TestRetireHistoryIsRefusedForARealHeldDelivery(t *testing.T) { } } -func TestOwedWorkTheForgeSaysCanNeverBeDoneIsDropped(t *testing.T) { +// What the bus cannot take is owed and tried again later each time, and given up after retryLimit tries, saying so. What the +// forge refuses is the bridge's to say (issue 1349): the gate has no retry for it. +func TestOwedWorkTheBusCannotTakeIsTriedAgainLaterAndGivenUpAfterRetryLimit(t *testing.T) { w := newWorld(t) id := IDOf("novox/app", head) - w.forge.viewErr = errors.New(`Gitea API /repos/novox/app/issues/7/comments: 500 {"message":"issue does not ` + - `exist [id: 0, repo_id: 53, index: 7]"}`) + w.owed.down = true w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) - w.h.Flush() - if w.forge.viewCalls != 1 { - t.Fatalf("the view was asked %d time(s)", w.forge.viewCalls) + tries := func() int { + for _, e := range w.delivery(id).Owed { + if e.Kind == EffectView { + return e.Tries + } + } + return -1 } - if d := w.delivery(id); len(d.Owed) != 0 { - t.Fatalf("an answer that its pull request does not exist left owed %+v", d.Owed) - } - w.settleAll() - if w.forge.viewCalls != 1 { - t.Fatalf("a dropped view was asked again: %d", w.forge.viewCalls) - } - if st := w.h.Status("listening"); !strings.Contains(strings.Join(st["refused"].([]string), "\n"), "given up") { - t.Fatalf("its drop was not said: %v", st["refused"]) - } - - // Any other failure is tried again later each time, and given up after retryLimit tries. - w.forge.viewErr = errors.New("the forge is away") - w.h.Checked(verdict("novox/app", 7, head, "pass")) - before := w.forge.viewCalls w.h.Flush() w.h.Flush() w.h.Flush() - if w.forge.viewCalls != before+1 { - t.Fatalf("a failed view was tried %d time(s) at once", w.forge.viewCalls-before) + if tries() != 1 { + t.Fatalf("a view the bus could not take was tried %d time(s) at once", tries()) } w.later(retryFirst + time.Second) w.h.Flush() - if w.forge.viewCalls != before+2 { - t.Fatalf("a failed view was not tried again after its wait: %d", w.forge.viewCalls-before) + if tries() != 2 { + t.Fatalf("a failed view was not tried again after its wait: %d", tries()) } if retryAfter(1) != retryFirst || retryAfter(2) != 2*retryFirst || retryAfter(100) != retryCap { t.Fatalf("the waits: %s %s %s", retryAfter(1), retryAfter(2), retryAfter(100)) @@ -220,14 +211,17 @@ func TestOwedWorkTheForgeSaysCanNeverBeDoneIsDropped(t *testing.T) { w.later(retryCap) w.h.Flush() } - for _, e := range w.delivery(id).Owed { - if e.Kind == EffectView { - t.Fatalf("a view failing %d times is still owed: %+v", e.Tries, e) - } + if tries() != -1 { + t.Fatalf("a view failing %d times is still owed", tries()) } - if !doesNotExist(errors.New("Gitea API /repos/a/b/issues/1/comments: 404 Not Found")) || - doesNotExist(errors.New("tool gitea.gitea_delivery_view does not exist on this machine")) || - doesNotExist(errors.New("the forge is away")) { - t.Fatal("doesNotExist reads the forge's answer wrong") + if st := w.h.Status("listening"); !strings.Contains(strings.Join(st["refused"].([]string), "\n"), "given up") { + t.Fatalf("its drop was not said: %v", st["refused"]) + } + // The bus back: what is owed anew is written. + w.owed.down = false + w.h.Checked(verdict("novox/app", 7, head, "pass")) + w.h.Flush() + if len(w.owed.acts(sdk.OwedView)) == 0 { + t.Fatal("nothing was written once the bus was back") } } 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 d96772ce..0f74eeec 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -33,6 +33,8 @@ type Holder struct { Operator Operator Merges MergeStore Source SourceStore + // Owed is the gate's bucket owed: what a transition owes the forge, written as keys (owed.go). + Owed OwedStore // Pulls keeps the pull requests' keys on the bus (pullkey.go); nil keeps none. Pulls PullKeys // Seat says where today's module still stands; the gate holds the seat only when it stands nowhere (seat.go). Carry @@ -1956,7 +1958,8 @@ func (h *Holder) askOwnCheck(id string, m Member, why string) { return } if asked == "" { - h.Logf("[mesh-delivery] %s's check was asked and the controller's answer held no id: taken as refused, asked again in a minute at most", id) + h.Logf("[mesh-delivery] ERROR: %s's check was asked and the controller's answer held no check id: it is not taken as asked, "+ + "and asked again in a minute at most", id) return } delete(h.askTried, id) 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 3f1c841b..92ad64d2 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 @@ -250,19 +250,19 @@ func TestATransitionIsKeptBeforeItIsSaid(t *testing.T) { if len(w.said) != 1 || len(w.forge.notesOn(head)) != 2 { t.Fatalf("once kept, said %d and noted %d", len(w.said), len(w.forge.notesOn(head))) } - // A forge that is away is asked again; a note is appended once however often it is asked. - w.forge.down = true + // A bucket the bus cannot take is written again; a note is appended once however often it is asked. + w.owed.down = true w.h.Checked(verdict("novox/app", 7, head, "pass")) w.settleAll() if len(w.forge.notesOn(head)) != 2 { - t.Fatal("noted while the forge was away") + t.Fatal("noted while the bus was away") } - w.forge.down = false + w.owed.down = false w.later(time.Hour) // each failure put its next try later: past every one of them w.settleAll() w.settleAll() if notes := w.forge.notesOn(head); len(notes) != 4 { - t.Fatalf("after the forge came back the note has %d line(s): %q", len(notes), notes) + t.Fatalf("after the bus came back the note has %d line(s): %q", len(notes), notes) } if d := w.delivery(IDOf("novox/app", head)); len(d.Owed) != 0 { t.Fatalf("still owed: %+v", d.Owed) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1349_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1349_test.go new file mode 100644 index 00000000..bb282ec2 --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1349_test.go @@ -0,0 +1,231 @@ +package main + +import ( + "encoding/json" + "fmt" + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "slices" + "strconv" + "strings" + "sync" + "testing" + + sdk "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// **The gate writes nothing to the forge: its statuses, views, notes and comments are owed keys, and its plan is read from +// `plans`** (novox/hq ADR 0437 §2, §8, ADR 0450 §2, §4, §7, issue 1349; the empty check answer is the mesh-delivery half of +// issue 1311). Fixture sources: the key and the value of an owed act are mesh-sdk go/delivery's (OwedKey, Owed); the arguments +// of `plans` are mesh-controller cmd/mesh-controller/seatverbs.go's (repository, paths, module-dirs, head, base, removed, data) +// and its answer the document of deliveryPlanDocument (plan, reach, mesh, gated). + +// AC1: a transition that today sets a status, a view, a note or a comment writes the matching owed key. +func TestATransitionWritesItsStatusViewAndNoteAsOwedKeys(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", head) + w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) + w.settleAll() + page := "https://forge.invalid/novox/app/pulls/7" + statusKey, _ := sdk.OwedKey("novox/app", 7, head, "status=mesh/delivery") + viewKey, _ := sdk.OwedKey("novox/app", 7, head, "view") + noteKey, _ := sdk.OwedKey("novox/app", 7, head, "note") + for _, key := range []string{statusKey, viewKey, noteKey} { + if _, there := w.owed.keys[key]; !there { + t.Fatalf("the key %s was not written; the keys are %v", key, w.owed.writes) + } + } + if s := w.owed.keys[statusKey]; s.Act != sdk.OwedStatus || s.Context != "mesh/delivery" || s.State != "pending" || s.Target != page || s.Text == "" { + t.Fatalf("the owed status is %+v; wanted mesh/delivery pending, pointing at %s", s, page) + } + if v := w.owed.keys[viewKey]; v.Act != sdk.OwedView || !strings.Contains(v.Text, id) { + t.Fatalf("the owed view is %+v", v) + } + if n := w.owed.keys[noteKey]; n.Act != sdk.OwedNote || n.Ref != "mesh-plan" || !strings.Contains(n.Text, "proposed") { + t.Fatalf("the owed note is %+v", n) + } + // What the bridge would perform is what the forge holds. + if len(w.forge.notesOn(head)) == 0 || !strings.HasPrefix(statusOn(w, "novox/app", head), "pending") { + t.Fatalf("the fake bridge did not perform what was owed: %v", w.forge.notesOn(head)) + } +} + +// AC1: a note on the merge commit, which no pull request names, is keyed with the number 0. +func TestANoteOnTheMergeCommitIsOwedWithNumberZero(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) + w.h.Checked(verdict("novox/app", 7, head, "pass")) + w.h.PullMerged(merged("novox/app", 7, head, merge)) + w.h.WalkMoved(w.ctl.put(aWalk("plan-1", "novox/app", merge, true))) + w.settleAll() + key, _ := sdk.OwedKey("novox/app", 0, merge, "note") + if n, there := w.owed.keys[key]; !there || n.Act != sdk.OwedNote { + t.Fatalf("no note was owed on the merge commit under %s; the keys are %v", key, w.owed.writes) + } +} + +// AC1: two comments on one head are two acts: the sequence number is in the act. +func TestTwoCommentsOnOneHeadAreTwoOwedKeys(t *testing.T) { + w := newWorld(t) + id := IDOf("novox/app", head) + w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) + w.h.mu.Lock() + d := w.h.deliveries[id] + w.h.sayOn(d, "the first thing said") + w.h.sayOn(d, "the second thing said") + w.h.mu.Unlock() + w.settleAll() + one, _ := sdk.OwedKey("novox/app", 7, head, "comment.1") + two, _ := sdk.OwedKey("novox/app", 7, head, "comment.2") + if w.owed.keys[one].Text != "the first thing said" || w.owed.keys[two].Text != "the second thing said" { + t.Fatalf("the comments are %+v and %+v", w.owed.keys[one], w.owed.keys[two]) + } + if len(w.forge.comments) != 2 { + t.Fatalf("the bridge would say %d comments: %v", len(w.forge.comments), w.forge.comments) + } +} + +// AC2: the gate's code calls no gitea write tool and no `delivery-plan`, and no controller verb beyond the ones its manifest +// names (ADR 0450 §7). Tests are left out: they name the words to prove them gone. +func TestTheGatesCodeCallsNoForgeWriteToolAndNoDeliveryPlan(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + forbidden := []string{"delivery-plan", "gitea_commit_status\"", "gitea_delivery_view", "gitea_note_append", "gitea_add_comment", + "gitea_merge_pull_request\"", "gitea_open_pulls_write"} + // The controller's verbs the gate calls: ADR 0450 §7. + allowed := []string{"deliver", "delivery-stop", "delivery-backups-again", "delivery-walks", "delivery-check", "forget-checks", "plans"} + called := map[string]bool{} + fset := token.NewFileSet() + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, f, nil, 0) + if err != nil { + t.Fatal(err) + } + ast.Inspect(file, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.BasicLit: + if x.Kind == token.STRING { + for _, word := range forbidden { + if strings.Contains(x.Value, word) { + t.Errorf("%s holds the string %s", f, x.Value) + } + } + } + case *ast.CallExpr: + if sel, ok := x.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "verb" && len(x.Args) > 0 { + if lit, ok := x.Args[0].(*ast.BasicLit); ok { + verb, _ := strconv.Unquote(lit.Value) + called[verb] = true + if !slices.Contains(allowed, verb) { + t.Errorf("%s calls the controller's verb %q", f, verb) + } + } + } + } + return true + }) + } + for _, verb := range []string{"plans", "delivery-check"} { + if !called[verb] { + t.Errorf("the gate calls no %s: the test reads nothing", verb) + } + } +} + +// stubController is a controller that answers every verb with one document, and keeps what it was asked. +type stubController struct { + mu sync.Mutex + keys []string + bodies []map[string]any + answer string +} + +func (s *stubController) ask(key string, body any) (json.RawMessage, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.keys = append(s.keys, key) + b, _ := body.(map[string]any) + s.bodies = append(s.bodies, b) + return json.RawMessage(s.answer), nil +} + +func (s *stubController) called(key string) []map[string]any { + s.mu.Lock() + defer s.mu.Unlock() + var out []map[string]any + for i, k := range s.keys { + if k == key { + out = append(out, s.bodies[i]) + } + } + return out +} + +// AC3: a delivery with no plan gets it from `plans`, asked as data, with the change's diffset. +func TestADeliveryWithNoPlanGetsItFromPlansAsData(t *testing.T) { + w := newWorld(t) + stub := &stubController{answer: `{"ok":true,"output":"","answer":{"asked":"check-1","held":true,"walks":[], +"plan":{"repository":"novox/app","base":"main","head":"` + head + `","moved":["app"],"summary":"builds app, sent to the node"}}}`} + w.h.Controller = seatController{ask: stub.ask} + w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) + c := verdict("novox/app", 7, head, "pass") + c.Plan = nil + w.h.Checked(c) + w.settleAll() + asked := stub.called("seat:mesh-controller.plans") + if len(asked) == 0 { + t.Fatalf("`plans` was not asked; the controller was asked %v", stub.keys) + } + if len(stub.called("seat:mesh-controller.delivery-plan")) != 0 { + t.Fatal("delivery-plan was asked") + } + got := asked[0] + if got["repository"] != "novox/app" || got["paths"] != "src/a.go" || got["data"] != "true" || got["head"] != head || got["base"] != "main" { + t.Fatalf("plans was asked %v; wanted the repository, the paths, the head, the base and data", got) + } + if d := w.delivery(IDOf("novox/app", head)); d.Plan == nil || d.Plan.Summary != "builds app, sent to the node" { + t.Fatalf("the delivery's plan is %+v", d.Plan) + } +} + +// AC4: a delivery-check answer that holds no id is an error said aloud, naming the delivery, and not taken as asked. +func TestAnEmptyDeliveryCheckAnswerIsAnErrorNamingTheDelivery(t *testing.T) { + stub := &stubController{answer: `{"ok":true,"output":"","answer":{}}`} + if id, err := (seatController{ask: stub.ask}).Check("", []Member{{ID: "novox/app@x"}}); err == nil || id != "" || + !strings.Contains(err.Error(), "no check id") { + t.Fatalf("an empty check answer gave %q and %v", id, err) + } + w := newWorld(t) + var said []string + var mu sync.Mutex + w.h.Logf = func(format string, a ...any) { + mu.Lock() + defer mu.Unlock() + said = append(said, strings.TrimSpace(fmt.Sprintf(format, a...))) + } + w.h.Controller = seatController{ask: stub.ask} + w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go")) + w.settleAll() + id := IDOf("novox/app", head) + mu.Lock() + defer mu.Unlock() + found := false + for _, line := range said { + if strings.Contains(line, id) && strings.Contains(line, "no check id") && !strings.Contains(line, "taken as refused") { + found = true + } + } + if !found { + t.Fatalf("no error naming %s and the empty answer was logged: %q", id, said) + } + if d := w.delivery(id); d.CheckAsk != nil { + t.Fatalf("an empty answer was kept as an ask: %+v", d.CheckAsk) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue407_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue407_test.go index 551744ab..f71d08df 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue407_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue407_test.go @@ -57,15 +57,15 @@ func TestDeliveryStatusCountsTheOwedEffectsTried(t *testing.T) { t.Fatalf("last try said %v, tried at %s", tried["last"], w.now.Format(time.RFC3339)) } - // The forge away: its effects fail and are counted failed; quiet passes try nothing more. - w.forge.down = true + // The bus away: its effects fail and are counted failed; quiet passes try nothing more. + w.owed.down = true w.later(time.Minute) w.h.Checked(verdict("novox/app", 7, head, "pass")) w.h.Flush() tried = triedSaid(t, w) failed := countOf(t, tried, "failed") if failed == 0 { - t.Fatalf("the forge away failed nothing: %v", tried) + t.Fatalf("the bus away failed nothing: %v", tried) } if countOf(t, tried, "tried") != countOf(t, tried, "done")+failed+countOf(t, tried, "given-up") { t.Fatalf("tries do not add up: %v", tried) @@ -80,7 +80,7 @@ func TestDeliveryStatusCountsTheOwedEffectsTried(t *testing.T) { } // The forge back, past every next try: the failed effects are done. - w.forge.down = false + w.owed.down = false w.later(time.Hour) w.h.Flush() tried = triedSaid(t, w) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go index 4588a811..e5f4348a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go @@ -49,6 +49,7 @@ func main() { Operator: kvOperator{}, Merges: kvMerges{}, Source: kvSource{}, + Owed: kvOwed{}, Pulls: kvPulls{}, // The gate acts only while it holds the seat, and carries today's state over when it first does (seat.go). Seat: busOccupancy{}, 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 863d832a..ada614f3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -38,6 +38,8 @@ type Delivery struct { // that was said, for this pull request. After []string `json:"after,omitempty"` AfterSaid bool `json:"after_said,omitempty"` + // Comments is how many comments this owner owed on the pull request at this head, which numbers the next. + Comments int `json:"comments,omitempty"` // Issues are the issues its title and body name ("issue 948"), shown to the operator who is asked to approve its merge. Issues []int `json:"issues,omitempty"` // Group is the delivery group it is in: its head branch name. Empty for a lone delivery. @@ -321,6 +323,9 @@ const ( EffectNote = "note" EffectView = "view" EffectStatus = "status" + // EffectComment is a comment on the pull request; its Context is the sequence number of the comment on the head, which + // keeps two comments on one head two acts (mesh-sdk go/delivery OwedCommentAct). + EffectComment = "comment" ) // FromForgesWord is whether the delivery was made from the forge's word at its merge: said by FromForge, or — diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/owed.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/owed.go new file mode 100644 index 00000000..2e636dbe --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/owed.go @@ -0,0 +1,117 @@ +package main + +import ( + "encoding/json" + "errors" + "fmt" + "strconv" + "strings" + + stdio "git.novox.be/novox/mesh-sdk/go" + sdk "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// The gate writes nothing to the forge: what a transition owes it — a status, the view on the pull request, a line of the +// commit's note, a comment — is one key of the gate's bucket owed, `...` (mesh-sdk go/delivery +// OwedKey), which the forge bridge performs once for each revision (novox/hq ADR 0437 §8, ADR 0450 §4). The number is 0 for +// a commit no pull request names; a comment's act carries a sequence number, so two comments on one head are two acts. +// What the gate keeps is the effect owed until its key is written (effects.go); the forge's own refusal is the bridge's +// to say on FORGE, and the gate has no retry for it. + +// noPage is the target of a status whose delivery has no page to point at. +const noPage = "none" + +// notesRef is the notes ref the commit's note is kept under. +const notesRef = "mesh-plan" + +// OwedStore is the gate's bucket owed. +type OwedStore interface { + // Put writes the act under its key, unless that key holds the same act already (a revision is an act to perform). + Put(key string, o sdk.Owed) error +} + +// kvOwed is owed over the runtime. +type kvOwed struct{} + +func (kvOwed) Put(key string, o sdk.Owed) error { + raw, err := sdk.EncodeOwed(o) + if err != nil { + return err + } + if e, err := stdio.State("owed").Get(key); err == nil && e != nil { + if kept, err := sdk.DecodeOwed(e.Value); err == nil && sdk.SameOwed(kept, o) { + return nil + } + } + _, err = stdio.State("owed").Put(key, json.RawMessage(raw)) + return err +} + +// owe writes one owed act as its key. An act that has no key (a name the key refuses, a status with no page to point at) is +// said and dropped, since writing it again would fail the same. +func (h *Holder) owe(repository string, number int, head, act string, o sdk.Owed) error { + if h.Owed == nil { + return errors.New("no owed bucket") + } + key, err := sdk.OwedKey(repository, number, head, act) + if err == nil { + o.Act = actKind(act) + _, err = sdk.EncodeOwed(o) + } + if err != nil { + h.Refuse(fmt.Errorf("the act %q on %s#%d at %s cannot be owed and is dropped: %v", act, repository, number, shortOf(head, 12), err)) + return nil + } + return h.Owed.Put(key, o) +} + +// actKind is the kind of an owed act: `status=` is a status, `comment.` a comment. +func actKind(act string) string { + kind, _, _ := sdk.ParseOwedAct(act) + return kind +} + +// statusAct is the act of a status of a context. +func statusAct(context string) string { return sdk.OwedStatus + "=" + context } + +// commentAct is the act of the seq-th comment on a head. +func commentAct(seq int) string { + act, _ := sdk.OwedCommentAct(seq) + return act +} + +// owedOf is an effect as the act its key names and its value: the repository and number are the delivery's (or, for a +// group's status, the member's the effect names in Last), the body the view's. +func owedOf(e Effect, repository string, number int, head, body, page string) (act string, value sdk.Owed, num int, hd string) { + num, hd = number, head + switch e.Kind { + case EffectNote: + hd = e.Commit + if e.Commit != head { + num = 0 // the merge commit is named by no pull request + } + return sdk.OwedNote, sdk.Owed{Text: e.Line, Ref: notesRef}, num, hd + case EffectView: + return sdk.OwedView, sdk.Owed{Text: body}, num, hd + case EffectStatus: + // The owed status must name where it points; a delivery with no pull request page points at none, which the forge's + // tool leaves out (it links only an http or https address). + if page == "" { + page = noPage + } + return statusAct(e.Context), sdk.Owed{Context: e.Context, State: e.State, Text: e.Line, Target: page}, num, e.Commit + case EffectComment: + seq, _ := strconv.Atoi(e.Context) + return commentAct(seq), sdk.Owed{Text: e.Line}, num, hd + } + return "", sdk.Owed{}, 0, "" +} + +// memberOfLast reads the repository, pull request number and page a group's status effect names in Last +// (`owner/repo#N page`). +func memberOfLast(last string) (repository string, number int, page string) { + where, page, _ := strings.Cut(last, " ") + repository, n, _ := strings.Cut(where, "#") + number, _ = strconv.Atoi(n) + return repository, number, page +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go index c4f1041d..4cb8767f 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go @@ -71,12 +71,6 @@ type Controller interface { // Forge is what the forge's holder does for a delivery. type Forge interface { - // Note appends one line to a commit's note under refs/notes/mesh-plan; a line already there is not added. - Note(owner, repo, commit, line string) error - // View keeps the delivery's view current on its pull request: one comment, edited in place. - View(owner, repo string, number int, body string) error - // Status sets one status of a commit, linking to the view. - Status(owner, repo, commit, context, state, description, target string) error // Statuses reads a commit's statuses — a pull request's head's, given its number — and whether the branch // it merges into would let it merge. Reads only. Statuses(owner, repo string, number int, commit, base string) (*ForgeChecks, error) @@ -85,8 +79,6 @@ type Forge interface { // OpenPulls is every open pull request, with what its base requires and what its head has (novox/hq issue // 347). Reads only. OpenPulls() ([]OpenPull, error) - // Comment adds a comment to a pull request. - Comment(owner, repo string, number int, body string) error } // ForgeChecks is what the forge says of a commit's checks (the gitea module's gitea_commit_statuses). @@ -206,8 +198,10 @@ func (c seatController) verb(name string, args map[string]any) (json.RawMessage, return answer, nil } +// Plan asks the controller's `plans` for the plan of a change as data (novox/hq ADR 0450 §2): the document the old +// per-change verb answered, now the what-if of the plans every person reads as text. func (c seatController) Plan(repository, base, head string, paths, moduleDirs, removed []string) (*DeliveryPlan, error) { - args := map[string]any{"repository": repository, "paths": strings.Join(paths, ",")} + args := map[string]any{"repository": repository, "paths": strings.Join(paths, ","), "data": "true"} if base != "" { args["base"] = base } @@ -220,7 +214,7 @@ func (c seatController) Plan(repository, base, head string, paths, moduleDirs, r if len(removed) > 0 { args["removed"] = strings.Join(removed, ",") } - raw, err := c.verb("delivery-plan", args) + raw, err := c.verb("plans", args) if err != nil { return nil, err } @@ -228,7 +222,7 @@ func (c seatController) Plan(repository, base, head string, paths, moduleDirs, r Plan *DeliveryPlan `json:"plan"` } if err := json.Unmarshal(raw, &a); err != nil || a.Plan == nil { - return nil, errors.New("the controller's delivery-plan answered no plan") + return nil, errors.New("the controller's plans answered no plan as data") } return a.Plan, nil } @@ -250,7 +244,10 @@ func (c seatController) Check(group string, members []Member) (string, error) { if a.Asked != "" { return a.Asked, nil } - return a.Rechecked, nil + if a.Rechecked != "" { + return a.Rechecked, nil + } + return "", fmt.Errorf("the controller's delivery-check answered no check id (%s): it is not taken as asked", clip(string(raw), 200)) } func (c seatController) Deliver(walk, why string) error { @@ -341,20 +338,6 @@ func (f toolForge) tool(name string, args map[string]any) error { return nil } -func (f toolForge) Note(owner, repo, commit, line string) error { - return f.tool("gitea_note_append", map[string]any{"owner": owner, "repo": repo, "sha": commit, - "ref": "mesh-plan", "line": line}) -} - -func (f toolForge) View(owner, repo string, number int, body string) error { - return f.tool("gitea_delivery_view", map[string]any{"owner": owner, "repo": repo, "number": number, "body": body}) -} - -func (f toolForge) Status(owner, repo, commit, context, state, description, target string) error { - return f.tool("gitea_commit_status", map[string]any{"owner": owner, "repo": repo, "sha": commit, - "context": context, "state": state, "description": description, "target_url": target}) -} - func (f toolForge) Statuses(owner, repo string, number int, commit, base string) (*ForgeChecks, error) { args := map[string]any{"owner": owner, "repo": repo} if number > 0 { @@ -381,10 +364,6 @@ func (f toolForge) Statuses(owner, repo string, number int, commit, base string) return &c, nil } -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}) -} - func (f toolForge) OpenPulls() ([]OpenPull, error) { raw, err := f.ask("gitea.gitea_open_pulls", map[string]any{}) if err != nil { 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 f5ff6be6..0fd35506 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -3,6 +3,7 @@ package main import ( "fmt" "sort" + "strconv" "strings" "time" @@ -343,14 +344,13 @@ func (h *Holder) groupTurnToMerge(d *Delivery) string { return "" } -// sayOn comments on a delivery's pull request. +// sayOn owes a comment on a delivery's pull request: an owed act numbered among the comments on its head, written as a key +// with the delivery's other effects (owed.go). Under the lock; what it returns has nothing left to do. func (h *Holder) sayOn(d *Delivery, body string) func() { - owner, repo, number, id := d.Owner(), d.Repo(), d.Number, d.ID - return func() { - if err := h.Forge.Comment(owner, repo, number, body); err != nil { - h.Logf("[mesh-delivery] %s: could not say on its pull request: %v", id, err) - } - } + d.Comments++ + d.Owed = append(d.Owed, Effect{Kind: EffectComment, Commit: d.Commit, Line: body, Context: strconv.Itoa(d.Comments), Since: h.Now()}) + h.keep(d) + return func() {} } // readyLines are the approved heads that will not merge: a check that failed at once, a merge refused past its bound; diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/recheck_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/recheck_test.go index 97c843c8..f8bc9be4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/recheck_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/recheck_test.go @@ -80,12 +80,12 @@ func TestARecheckLeavesAStatusTheHeadNeverHadAlone(t *testing.T) { } } -// A forge that cannot be told refuses the recheck whole: nothing is asked on a stale verdict, and the +// A bus that cannot take the pending statuses refuses the recheck whole: nothing is asked on a stale verdict, and the // delivery stays where it stood, for the person to ask again. -func TestARecheckTheForgeCannotTakeIsRefused(t *testing.T) { +func TestARecheckTheBusCannotTakeIsRefused(t *testing.T) { w := newWorld(t) id := heldGreen(t, w, "novox/app", 9) - w.forge.down = true + w.owed.down = true if _, err := w.h.Recheck(id, "main moved on", "a person"); err == nil { t.Fatal("a recheck whose statuses could not be put back was taken") } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/timers_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/timers_test.go index 370d1127..d1d6214e 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/timers_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/timers_test.go @@ -109,16 +109,16 @@ func TestAMergeNoWalkOpenedForReadsTheControllersWalksOnceAtItsBound(t *testing. } } -func TestAnEffectTheForgeCouldNotTakeIsTriedAgainAtItsTimeByTheTimerAlone(t *testing.T) { +func TestAnEffectTheBusCouldNotTakeIsTriedAgainAtItsTimeByTheTimerAlone(t *testing.T) { w := newWorld(t) - w.forge.down = true + w.owed.down = true w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.React() at, step := w.wantedAt() if at.IsZero() || !strings.Contains(step, "owed") || at.After(w.now.Add(retryFirst)) { t.Fatalf("the look wanted at %s for %q; wanted within %s for the owed effect", at, step, retryFirst) } - w.forge.down = false + w.owed.down = false w.later(retryFirst + time.Second) w.h.React() if len(w.forge.notes) == 0 { 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 9a3e5aaf..b7bee361 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go @@ -297,9 +297,9 @@ func (h *Holder) Recheck(id, why, by string) (string, error) { h.mu.Unlock() return "", err } - owner, repo, commit, page := d.Owner(), d.Repo(), d.Commit, d.HTMLURL + owner, repo, commit, page, number := d.Owner(), d.Repo(), d.Commit, d.HTMLURL, d.Number h.mu.Unlock() - if err := h.checkingAgain(owner, repo, commit, page, why); err != nil { + if err := h.checkingAgain(owner, repo, number, commit, page, why); err != nil { return "", fmt.Errorf("%s is not rechecked: %v; its merge check must say pending before it is asked again, "+ "so nothing merges on the old verdict — ask again once the forge answers", id, err) } @@ -336,10 +336,11 @@ func (h *Holder) Recheck(id, why, by string) (string, error) { // verdict sets it; the repository's own check only when the head holds one, since a verdict that says none // would leave a pending one standing for ever. Asked of the forge's holder, which keeps them; it takes // pending on them and nothing else, a verdict being the controller's alone (novox/hq issue 293). -func (h *Holder) checkingAgain(owner, repo, commit, page, why string) error { +func (h *Holder) checkingAgain(owner, repo string, number int, commit, page, why string) error { if h.Forge == nil { return errors.New("no forge") } + // The statuses are owed as keys the forge bridge performs, on the pull request number the delivery has. contexts := []string{ctxGate, ctxRepo} if held, err := h.Forge.Statuses(owner, repo, 0, commit, ""); err == nil { contexts = []string{ctxGate} @@ -351,8 +352,10 @@ func (h *Holder) checkingAgain(owner, repo, commit, page, why string) error { } line := clip(oneLine("checking again: "+why), 140) for _, c := range contexts { - if err := h.Forge.Status(owner, repo, commit, c, "pending", line, page); err != nil { - return fmt.Errorf("the forge did not set %s pending on %s: %w", c, shortOf(commit, 8), err) + act, value, num, head := owedOf(Effect{Kind: EffectStatus, Commit: commit, Context: c, State: "pending", Line: line}, + owner+"/"+repo, number, commit, "", page) + if err := h.owe(owner+"/"+repo, num, head, act, value); err != nil { + return fmt.Errorf("%s could not be owed as pending on %s: %w", c, shortOf(commit, 8), err) } } return nil