diff --git a/modules/mesh-delivery-gate/README.md b/modules/mesh-delivery-gate/README.md index 66299ef3..a7ed14ad 100644 --- a/modules/mesh-delivery-gate/README.md +++ b/modules/mesh-delivery-gate/README.md @@ -36,6 +36,14 @@ names every refusal and rejection at once, and the pull request is told at once; `needs: new-head` and names its author: the branch, and the session a `Pushed-by: ` line in the pull request's description names. +## A merge the forge account cannot carry out is not asked (issue 1945) + +Before the operator's Approve is asked, the gate reads through `gitea_commit_statuses` whether the forge's merging +account writes each repository the ask covers (`merge.writes`). Where it does not, nothing is asked: the delivery waits +on "the forge account cannot write : gitea_grant_write" (`show`), raised at once as a condition. The read holds +until the gitea module's `gitea.repo.writable` (its grant, read back) or ten minutes, then the ask is made again; an +approved merge of that repository the forge refused is asked again on the same event (`writes.go`). + ## The seat's hand-over The gate took the seat `mesh-delivery` over from the module `mesh-delivery` (ADR 0449 §4, ADR 0450 §6, ADR 0454) and carried its diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go index 3229d6d8..3c385094 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -339,6 +339,9 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { return "the operator cannot be asked from here", nil } members := h.askMembers(d) + if waits := h.writeWaits(d, members, now); waits != "" { + return waits, nil // asked only once the forge account can carry the merge out (writes.go, issue 1945) + } group := "" if len(members) > 1 { group = d.Group @@ -423,6 +426,16 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { h.mu.Unlock() } } + // Nothing is asked that the forge account could not merge (writes.go, issue 1945): said at the next look. + if says := h.cannotWrite(reads); says != "" { + stale() + h.mu.Lock() + defer h.mu.Unlock() + undo() + h.Logf("[mesh-delivery] %s passed its checks and the operator is not asked: %s", d.ID, says) + h.lookAt(h.Now(), "an ask not made: the forge account cannot write a repository it covers") + return + } if readErr != nil || holdSays != "" { stale() h.mu.Lock() 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 6bbec874..f1d36f37 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 @@ -101,7 +101,9 @@ type fakeForge struct { viewCalls int // What Statuses reads: the pull requests by owner/repo#number, the base's required statuses, whether the // protection cannot be read, and what it was asked. - pulls map[string]ForgePull + pulls map[string]ForgePull + // unwritable are the repositories (owner/repo) the forge's merging account cannot write (issue 1945). + unwritable map[string]bool required []string protectionDown bool asked []string @@ -220,6 +222,10 @@ func (f *fakeForge) Statuses(owner, repo string, number int, commit, base string base = "main" } out.Merge = ForgeMerge{Branch: base, Protected: f.required != nil, Required: f.required} + if f.unwritable[owner+"/"+repo] { + no := false + out.Merge.Writes, out.Merge.CannotWrite = &no, "the forge account cannot write "+owner+"/"+repo+": gitea_grant_write" + } if f.protectionDown { out.Merge.Says = "the protection of " + base + " could not be read" return out, nil 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 06b56023..138c7589 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -82,6 +82,8 @@ type Holder struct { // unmergeableSince is when the forge first said a group's member does not merge now, by group and member (issue 1275, // group_merge.go): within forgeMergeGrace that is the forge still working out mergeability, asked again shortly. unmergeableSince map[string]time.Time + // writeReads is whether the forge's merging account writes each repository, as last read (writes.go, issue 1945). + writeReads map[string]*writeRead // Turns is the gate's bucket turns (turns.go, ADR 0548 §3); nil keeps none. turnsHeld are the turns as written there, // by repository, read once at the first look (turnsLoaded). Turns TurnStore 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 58ff5b46..be04c7f1 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go @@ -34,7 +34,7 @@ func (l *listening) get() string { l.mu.Lock(); defer l.mu.Unlock(); return l.n // The events this owner follows: the forge's pull requests. The forge's status events say that a check answered (events.go). var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", "gitea.forge.status", "gitea.forge.pull-request.edited", "gitea.forge.pull-request.closed", - forgePattern, ConfigChangedEvent} + forgePattern, ConfigChangedEvent, writableEvent} // forgePattern is the forge bridge's messages on FORGE (novox/hq ADR 0437 §8): the gate follows a merge and the refusal of one. const forgePattern = "mesh-delivery-forge-bridge.forge.**" @@ -219,6 +219,10 @@ func take(h *Holder, e stdio.Envelope) error { } h.CommitStatus(s) h.KeepCheckVerdict(s) + case writableEvent: + if err := h.WriteGranted(e.Body); err != nil { + h.Refuse(err) + } case ConfigChangedEvent: var c ConfigChange if err := json.Unmarshal(e.Body, &c); err != nil { 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 a18d5cab..76b40c28 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ports.go @@ -76,6 +76,10 @@ type ForgeMerge struct { } `json:"blocking"` Mergeable *bool `json:"mergeable"` Says string `json:"says"` + // Writes is whether the forge's merging account may write the repository at all (issue 1945): false, its statuses and + // its merge are refused, and CannotWrite says so with the tool that grants it. Nil when the forge holder did not say. + Writes *bool `json:"writes,omitempty"` + CannotWrite string `json:"cannot_write,omitempty"` } // Store keeps deliveries and groups: one key each, in the module's own state (ADR 0201). 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 cf2d5e5b..4b96bc11 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -128,6 +128,11 @@ type ReadyRun struct { HeldSince *time.Time `json:"held_since,omitempty"` HeldFor string `json:"held_for,omitempty"` HandNow string `json:"hand_now,omitempty"` + + // CannotWrite is why the operator is not asked: the forge's merging account cannot write a repository the ask covers + // (writes.go, issue 1945); CannotWriteSince since when. Said at once as a condition. + CannotWrite string `json:"cannot_write,omitempty"` + CannotWriteSince *time.Time `json:"cannot_write_since,omitempty"` } // MergeBranch is the one branch this owner merges into (novox/hq ADR 0398 §4): a merge into any other is its author's. @@ -520,6 +525,12 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), 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.CannotWrite != "" && r.Approved == nil && r.CannotWriteSince != nil: + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, + For: now.Sub(*r.CannotWriteSince).Round(time.Second).String(), Bound: "0s", + H2: "none: the forge account given write on the repository (gitea_grant_write; the gitea module grants it itself once a module of it is registered), which asks the operator then", + Says: fmt.Sprintf("%s#%d passed its checks at %s and the operator is not asked for its Approve: %s", + d.Repository, d.Number, shortOf(r.Head, 12), r.CannotWrite)}) case r.Failed != "" && d.State == Rejected: // 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). diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes.go new file mode 100644 index 00000000..cb142cbe --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes.go @@ -0,0 +1,115 @@ +package main + +import ( + "encoding/json" + "fmt" + "strings" + "time" +) + +// **The operator is asked only for a merge the forge account can carry out** (novox/hq issue 1945). car-hunter #2, a +// repository of a user, was approved, and its first act after the Approve, the commit status, was refused 403: the mesh's +// merging account (the gitea module's tools, mesh-tools) only read that repository. Before the gate asks the operator's +// Approve it reads, through the forge holder's gitea_commit_statuses, whether that account writes each repository the +// ask covers (merge.writes), as the ask is about to be written. Where it does not, nothing is asked: the delivery waits, +// saying "the forge account cannot write : gitea_grant_write", and the gate raises it as a condition at once. +// +// A repository not written is kept so, and the ask is not composed again, until the gitea module's event +// gitea.repo.writable (its grant, read back from the forge) or, as a safety net, writeReadLasts after the read. A forge +// that does not say merge.writes (an older gitea module) is taken as writing: the merge says its own refusal, as before. + +// writeReadLasts is how long a read of whether the forge account writes a repository holds. +const writeReadLasts = 10 * time.Minute + +// writableEvent is the gitea module's event of an account given write on a repository. +const writableEvent = "gitea.repo.writable" + +// writeRead is what the forge last said of the merging account on one repository it cannot write. +type writeRead struct { + says string + at time.Time +} + +// cannotWriteSays is what a delivery waits on when the forge account cannot write its repository. +func cannotWriteSays(repository string) string { + return "the forge account cannot write " + repository + ": gitea_grant_write" +} + +// writeWaits is what an ask about to be made waits on: a repository it covers that the forge account was read, within +// writeReadLasts, not to write. Under the lock. +func (h *Holder) writeWaits(d *Delivery, members []*Delivery, now time.Time) string { + r := d.Ready + for _, m := range members { + repo := strings.ToLower(m.Repository) + w := h.writeReads[repo] + if w == nil { + continue + } + if !now.Before(w.at.Add(writeReadLasts)) { + delete(h.writeReads, repo) // read again at the ask + continue + } + if r.CannotWrite != w.says { + r.CannotWrite, r.CannotWriteSince = w.says, &now + h.keep(d) + } + h.lookAt(w.at.Add(writeReadLasts), "a repository the forge account could not write, read again") + return w.says + } + if r.CannotWrite != "" { + r.CannotWrite, r.CannotWriteSince = "", nil + h.keep(d) + } + return "" +} + +// cannotWrite reads, at the ask, whether the forge account writes each repository the ask covers, and answers what the +// first one it cannot write says; a read that fails says nothing (the merge says its own refusal, as before). Outside the +// lock; a repository not written is kept, under the lock. +func (h *Holder) cannotWrite(reads []memberRead) string { + for _, m := range reads { + fc, err := h.Forge.Statuses(m.owner, m.repo, m.number, m.head, m.base) + if err != nil || fc == nil || fc.Merge.Writes == nil || *fc.Merge.Writes { + continue + } + repo := strings.ToLower(m.owner + "/" + m.repo) + says := fc.Merge.CannotWrite + if says == "" { + says = cannotWriteSays(repo) + } + h.mu.Lock() + if h.writeReads == nil { + h.writeReads = map[string]*writeRead{} + } + h.writeReads[repo] = &writeRead{says: says, at: h.Now()} + h.mu.Unlock() + return says + } + return "" +} + +// WriteGranted takes the gitea module's gitea.repo.writable: the repository's read is dropped, so the next look reads it +// again, and an approved merge of it that the forge refused is asked again, its cause gone. +func (h *Holder) WriteGranted(body json.RawMessage) error { + var e struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + } + if err := json.Unmarshal(body, &e); err != nil || e.Owner == "" || e.Repo == "" { + return fmt.Errorf("%s names no repository: %s", writableEvent, string(body)) + } + repo := strings.ToLower(e.Owner + "/" + e.Repo) + h.mu.Lock() + defer h.mu.Unlock() + delete(h.writeReads, repo) + for _, d := range h.deliveries { + if strings.ToLower(d.Repository) != repo || d.Ready == nil || d.Ready.Approved == nil || d.Ready.Merged != nil { + continue + } + d.Ready.Repeats = 0 + h.mergeAgainNow(d) + h.keep(d) + } + h.Logf("[mesh-delivery] the forge account was given write on %s: it is read again before an ask, and an approved merge of it is asked again", repo) + return nil +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes_test.go new file mode 100644 index 00000000..28624230 --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/writes_test.go @@ -0,0 +1,44 @@ +package main + +import ( + "strings" + "testing" +) + +// Issue 1945: car-hunter #2 was asked, approved, and its commit status refused 403, since the forge's merging account only +// read the repository. 1945/AC3: a pull request whose checks passed, in a repository the merging account cannot write, is +// not asked; it waits saying so, loud as a condition at once. 1945/AC4: the gitea module's gitea.repo.writable asks it then. +func TestAnUnwritableRepositoryIsNotAskedAndSaysSoUntilTheWriteIsGranted(t *testing.T) { + w := announced(t) + conds := hearConditions(w) + w.forge.unwritable = map[string]bool{"novox/app": true} + hear(t, w, shimCheckedKey, judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.h.React() + if w.op.count() != 0 || len(w.op.open()) != 0 { + t.Fatalf("the operator was asked %d times for a merge the forge account cannot carry out", w.op.count()) + } + d := w.h.deliveries[IDOf("novox/app", readyHead)] + want := "the forge account cannot write novox/app: gitea_grant_write" + if d.Ready.Waits != want || d.Ready.CannotWrite != want { + t.Fatalf("waits %q, cannot_write %q; wanted %q", d.Ready.Waits, d.Ready.CannotWrite, want) + } + if c := conds.last(d.ID); c == nil || c.State != "raised" || !strings.Contains(c.Summary+c.Explanation+c.Headline, "cannot write novox/app") { + t.Fatalf("not said as a condition: %+v", c) + } + // Looked at again with nothing changed, the forge is not read again: the read holds until the grant or its bound. + asked := len(w.forge.asked) + w.h.React() + if len(w.forge.asked) != asked || w.op.count() != 0 { + t.Fatalf("read again %d times, asked %d", len(w.forge.asked)-asked, w.op.count()) + } + + delete(w.forge.unwritable, "novox/app") + hear(t, w, writableEvent, map[string]string{"owner": "novox", "repo": "app", "account": "mesh-tools"}) + w.h.React() + if w.op.count() != 1 { + t.Fatalf("after the write was granted the operator was asked %d times, wanted once", w.op.count()) + } + if d.Ready.CannotWrite != "" || strings.Contains(d.Ready.Waits, "cannot write") { + t.Fatalf("still says it cannot write: %q / %q", d.Ready.CannotWrite, d.Ready.Waits) + } +} diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index afe42fc1..6dd019be 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -26,7 +26,8 @@ "gitea.forge.pull-request.edited", "gitea.forge.pull-request.closed", "mesh-delivery-forge-bridge.forge.**", - "mesh-config.config-changed" + "mesh-config.config-changed", + "gitea.repo.writable" ], "emits": [ "transition",