From 5e24deb0f1977067a04ad2358cf1fd70d7b45204 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 17:27:15 +0200 Subject: [PATCH] gitea: require mesh/check in place of the two old statuses on every main the gate judges (issue 1301, S1-S3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0447 §1 has main require mesh/check alone. The repositories program now requires it in place of mesh/merge-gate and mesh/repo-check on every listed repository and on every other one whose main requires mesh/merge-gate, so the switch needs no hand-kept list (lead decisions L4, L11). A main requiring mesh/repo-check alone (hq) keeps it until issue 1296 (ADR 0503 §5). --- modules/gitea/README.md | 18 +- modules/gitea/cmd/repositories/check_test.go | 215 ++++++++++++++++++ modules/gitea/cmd/repositories/fake_test.go | 38 +++- modules/gitea/cmd/repositories/first.go | 6 +- modules/gitea/cmd/repositories/main.go | 3 +- .../gitea/cmd/repositories/override_test.go | 8 +- .../gitea/cmd/repositories/repositories.go | 154 ++++++++++--- .../cmd/repositories/repositories_test.go | 41 ++-- modules/gitea/module.json | 2 +- 9 files changed, 410 insertions(+), 75 deletions(-) create mode 100644 modules/gitea/cmd/repositories/check_test.go diff --git a/modules/gitea/README.md b/modules/gitea/README.md index b9b8482c..79f93452 100644 --- a/modules/gitea/README.md +++ b/modules/gitea/README.md @@ -22,8 +22,8 @@ Every new head of an open pull request is announced as `pull.updated`, once, and minutes, or when the search fails. - **A pull request nobody checks is said — by the delivery's owner, not here.** `gitea_open_pulls` answers every open pull request from the forge itself, with the statuses its base requires and the ones its head has. The - `mesh-delivery` module asks it every five minutes, and a head on a base requiring `mesh/merge-gate` or - `mesh/repo-check` with neither for ten minutes since it first saw it so is a delivery held past its bound + delivery gate asks it every five minutes, and a head on a base requiring `mesh/check` (or hq's + `mesh/repo-check`) with none of them for ten minutes since it first saw it so is a delivery held past its bound (`delivery..stalled`, state `unannounced`), which the controller raises like any other (issue 347). Each of the head's statuses comes with its state and when it was set, so a head whose check started and never answered — a required check pending, or never set beside one that is — is said as a stalled line in the state @@ -154,15 +154,19 @@ listed repository: naming the description, and a `merge-check.sh` declaring `# mesh-check-toolchain: go` that vets and runs the unit tests, then the integration tests (`-tags integration`), of every Go module in the tree, and says so and passes on a tree with none. The rule is then narrowed to main's rule of ADR 0398 §2 (no push, only `mesh-tools` merges), - requiring `mesh/repo-check` and `mesh/merge-gate`, and only then is the repository moved into the organisation. + requiring `mesh/check` (ADR 0447 §1, issue 1301), and only then is the repository moved into the organisation. There main's rule is read back and put right once more (the step `rule-after-move`): the forge drops from a merge list a user without access to the repository, and under the setup account `mesh-tools` has none; - one found still under the setup account is finished: the steps it lacks are done, main is never written twice; -- one in the organisation is only put right: main's rule narrowed, a missing one of the two statuses added and every - status already there kept. +- one in the organisation is only put right: main's rule narrowed, `mesh/check` required in place of + `mesh/merge-gate` and `mesh/repo-check`, and every other status already there kept. A main that requires + `mesh/repo-check` without `mesh/merge-gate` is judged by its own script alone (hq, ADR 0503 §5, until issue 1296) + and keeps it; `mesh/check` is never dropped. -A step the forge refuses stops that repository, and the others go on. A repository taken out of the value is never -asked about: nothing is deleted, renamed, moved out or unprotected. The pass keeps what it did per repository in +Every other repository of the organisation whose main requires `mesh/merge-gate` has the two old statuses swapped +for `mesh/check`, and only its statuses (issue 1301): the switch reaches every repository the merge gate judged, with +no list kept by hand. A step the forge refuses stops that repository, and the others go on. Beyond that swap, a +repository taken out of the value is never changed: nothing is deleted, renamed, moved out or unprotected. The pass keeps what it did per repository in `repositories-outcome.json`. **How a refusal is seen.** `repositories sync` is a run-once process, declared as the module's last resource, so diff --git a/modules/gitea/cmd/repositories/check_test.go b/modules/gitea/cmd/repositories/check_test.go new file mode 100644 index 00000000..0a5dd7aa --- /dev/null +++ b/modules/gitea/cmd/repositories/check_test.go @@ -0,0 +1,215 @@ +package main + +import ( + "strings" + "testing" +) + +// The tests of issue 1301 (ADR 0447 §1, §5; ADR 0503 §3, §5; lead decisions L4 and L11): main requires `mesh/check` +// alone in place of `mesh/merge-gate` and `mesh/repo-check`; a main that requires `mesh/repo-check` without +// `mesh/merge-gate` (hq) keeps it; every repository of the organisation whose main requires `mesh/merge-gate` is +// switched, listed in the value or not; no other status is dropped. + +func narrowedRule(contexts ...any) map[string]any { + return map[string]any{ + "enable_push": false, "enable_push_whitelist": false, "push_whitelist_usernames": []any{}, + "enable_merge_whitelist": true, "merge_whitelist_usernames": []any{tools}, + "enable_status_check": true, "status_check_contexts": contexts, + "block_admin_merge_override": true, + } +} + +// 1301-S1: a listed repository whose main requires the two old statuses is put right to `mesh/check` in their place, +// every other status kept; a second pass changes nothing. +func TestListedRepositorySwapsTheTwoStatusesForCheck(t *testing.T) { + f := newFake() + repo := f.add("novox/mesh-delivery", map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate", "mesh/repo-check", "other/x")}, "main") + s, _ := syncer(t, f) + o := run(t, s, Value{"mesh-delivery": {}}) + got := strs(repo.Rules[mainRule]["status_check_contexts"]) + if strings.Join(got, ",") != "other/x,mesh/check" { + t.Errorf("main requires %v, want other/x and mesh/check", got) + } + if r := result(o, "mesh-delivery"); r.Did != DidPutRight { + t.Errorf("outcome %+v, want put right", r) + } + before := len(f.writes()) + run(t, s, Value{"mesh-delivery": {}}) + if w := f.writes(); len(w) != before { + t.Errorf("a second pass wrote %v", w[before:]) + } +} + +// 1301-S1: a repository the program makes requires `mesh/check` alone from its first rule in the organisation on. +func TestNewRepositoryRequiresCheckAlone(t *testing.T) { + f := newFake() + s, _ := syncer(t, f) + run(t, s, Value{"fresh": {Description: "d"}}) + rule := f.repos["novox/fresh"].Rules[mainRule] + if got := strs(rule["status_check_contexts"]); rule["enable_status_check"] != true || strings.Join(got, ",") != CheckStatus { + t.Errorf("a new repository's main requires %v, want %s alone", got, CheckStatus) + } + if readme := f.repos["novox/fresh"].Files["README.md"]; !strings.Contains(readme, CheckStatus) || + strings.Contains(readme, "mesh/merge-gate") || strings.Contains(readme, "mesh/repo-check") { + t.Errorf("the README names the statuses of before: %q", readme) + } +} + +// 1301-S1: mesh/check is never dropped, and a rule that requires it beside a retired status loses the retired one. +func TestCheckIsKeptAndARetiredStatusBesideItGoes(t *testing.T) { + f := newFake() + both := f.add("novox/both", map[string]map[string]any{mainRule: narrowedRule("mesh/check", "mesh/repo-check")}, "main") + right := f.add("novox/right", map[string]map[string]any{mainRule: narrowedRule("mesh/check")}, "main") + s, _ := syncer(t, f) + o := run(t, s, Value{"both": {}, "right": {}}) + if got := strs(both.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("both requires %v, want %s alone", got, CheckStatus) + } + if got := strs(right.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("right requires %v", got) + } + if result(o, "right").Did != DidUnchanged || result(o, "both").Did != DidPutRight { + t.Errorf("outcome %+v", o) + } +} + +// 1301-S1: a listed repository whose main requires nothing is given mesh/check, not the old two. +func TestListedRuleRequiringNothingGetsCheck(t *testing.T) { + f := newFake() + none := f.add("novox/none", map[string]map[string]any{mainRule: {"enable_push": false}}, "main") + s, _ := syncer(t, f) + run(t, s, Value{"none": {}}) + if got := strs(none.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("none requires %v, want %s", got, CheckStatus) + } +} + +// 1301-S2 (ADR 0503 §5): a main that requires mesh/repo-check without mesh/merge-gate is judged by its own script +// alone (hq): it keeps mesh/repo-check and is not given mesh/check, listed or not. +func TestOwnCheckAloneIsKept(t *testing.T) { + f := newFake() + hq := f.add("novox/hq", map[string]map[string]any{mainRule: narrowedRule("mesh/repo-check")}, "main") + lab := f.add("novox/mesh-lab", map[string]map[string]any{mainRule: narrowedRule("mesh/repo-check")}, "main") + s, _ := syncer(t, f) + o := run(t, s, Value{"hq": {}}) + for name, repo := range map[string]*fRepo{"hq": hq, "mesh-lab": lab} { + if got := strs(repo.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != "mesh/repo-check" { + t.Errorf("%s requires %v, want mesh/repo-check alone", name, got) + } + } + if result(o, "hq").Did != DidUnchanged { + t.Errorf("hq outcome %+v, want unchanged", result(o, "hq")) + } + for _, w := range f.writes() { + t.Errorf("a repository judged by its own check alone was written: %s", w) + } +} + +// 1301-S3 (L4, L11): a repository the value does not list whose main requires mesh/merge-gate has the two old +// statuses swapped for mesh/check, and only its statuses: nothing else of its rule is sent. One that requires +// neither, or mesh/repo-check alone, is not written to. +func TestUnlistedRepositoryRequiringTheGateIsSwitched(t *testing.T) { + f := newFake() + catalog := f.add("novox/mesh-catalog", map[string]map[string]any{mainRule: { + "enable_push": true, "enable_status_check": true, "status_check_contexts": []any{"mesh/merge-gate", "mesh/repo-check"}, + }}, "main") + toolsGo := f.add("novox/mesh-tools-go", map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate")}, "main") + f.add("novox/plain", map[string]map[string]any{mainRule: {"enable_push": false}}, "main") + f.add("novox/hq", map[string]map[string]any{mainRule: narrowedRule("mesh/repo-check")}, "main") + f.add("novox/norule", nil, "main") + f.add("mesh-admin/elsewhere", map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate")}, "main") + s, _ := syncer(t, f) + o := run(t, s, Value{}) + + if got := strs(catalog.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("mesh-catalog requires %v, want %s", got, CheckStatus) + } + if catalog.Rules[mainRule]["enable_push"] != true { + t.Errorf("an unlisted repository's rule was narrowed beyond its statuses: %v", catalog.Rules[mainRule]) + } + if got := strs(toolsGo.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("mesh-tools-go requires %v, want %s", got, CheckStatus) + } + want := []string{ + "PATCH /api/v1/repos/novox/mesh-catalog/branch_protections/main", + "PATCH /api/v1/repos/novox/mesh-tools-go/branch_protections/main", + } + if got := f.writes(); strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Errorf("writes:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } + for _, n := range []string{"mesh-catalog", "mesh-tools-go"} { + if r := result(o, n); r.Did != DidPutRight { + t.Errorf("%s outcome %+v, want put right", n, r) + } + } + for _, n := range []string{"plain", "hq", "norule", "elsewhere"} { + if r := result(o, n); r.Name != "" { + t.Errorf("%s is in the outcome though nothing was done: %+v", n, r) + } + } + before := len(f.writes()) + run(t, s, Value{}) + if w := f.writes(); len(w) != before { + t.Errorf("a second pass wrote %v", w[before:]) + } +} + +// 1301-S3: the organisation's repositories are read page by page, so one past the first page is switched too. +func TestUnlistedRepositoriesAreReadPastTheFirstPage(t *testing.T) { + f := newFake() + f.pageSize = 2 + for _, n := range []string{"a", "b", "c", "d", "e"} { + f.add("novox/"+n, map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate", "mesh/repo-check")}, "main") + } + s, _ := syncer(t, f) + run(t, s, Value{}) + for _, n := range []string{"a", "b", "c", "d", "e"} { + if got := strs(f.repos["novox/"+n].Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { + t.Errorf("%s requires %v", n, got) + } + } +} + +// 1301-S3: a refused swap is said with the repository and its step, and the pass goes on to the next. +func TestRefusedSwapIsSaid(t *testing.T) { + f := newFake() + f.add("novox/a", map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate")}, "main") + f.add("novox/b", map[string]map[string]any{mainRule: narrowedRule("mesh/merge-gate")}, "main") + f.refuse = func(method, path string) (int, string) { + if method == "PATCH" && strings.Contains(path, "/novox/a/") { + return 403, "no" + } + return 0, "" + } + s, _ := syncer(t, f) + o := run(t, s, Value{}) + if r := result(o, "a"); r.Did != DidRefused || r.Step != StepSwap { + t.Errorf("a: %+v, want refused at %s", r, StepSwap) + } + if r := result(o, "b"); r.Did != DidPutRight { + t.Errorf("b: %+v, want put right", r) + } +} + +// 1301-S3: the organisation's list refused is said, and the listed repositories are still done. +func TestOrganisationListRefusedIsSaid(t *testing.T) { + f := newFake() + f.refuse = func(method, path string) (int, string) { + if strings.HasPrefix(path, "/api/v1/orgs/") { + return 500, "down" + } + return 0, "" + } + s, _ := syncer(t, f) + o := run(t, s, Value{"x": {}}) + if result(o, "x").Did != DidMade { + t.Errorf("x: %+v", result(o, "x")) + } + refusedList := false + for _, r := range o.Refused() { + refusedList = refusedList || r.Step == StepList + } + if !refusedList { + t.Errorf("the refused list is not in the outcome: %+v", o) + } +} diff --git a/modules/gitea/cmd/repositories/fake_test.go b/modules/gitea/cmd/repositories/fake_test.go index f78be551..b265057d 100644 --- a/modules/gitea/cmd/repositories/fake_test.go +++ b/modules/gitea/cmd/repositories/fake_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "strings" "sync" "testing" @@ -43,6 +44,8 @@ type fakeForge struct { calls []string // "METHOD /path" // refuse answers a call with a status and a message when it returns a status above zero. refuse func(method, path string) (int, string) + // pageSize is how many repositories one page of the organisation's list holds; 50 when zero. + pageSize int } func newFake() *fakeForge { return &fakeForge{repos: map[string]*fRepo{}} } @@ -84,6 +87,7 @@ var ( branchPath = regexp.MustCompile(`^/api/v1/repos/([^/]+)/([^/]+)/branches/([^/]+)$`) filesPath = regexp.MustCompile(`^/api/v1/repos/([^/]+)/([^/]+)/contents$`) movePath = regexp.MustCompile(`^/api/v1/repos/([^/]+)/([^/]+)/transfer$`) + orgRepos = regexp.MustCompile(`^/api/v1/orgs/([^/]+)/repos$`) ) func strs(v any) []string { @@ -167,6 +171,31 @@ func (f *fakeForge) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch { case p == "/api/v1/version": reply(200, map[string]string{"version": "1.27"}) + case orgRepos.MatchString(p) && r.Method == http.MethodGet: + owner := orgRepos.FindStringSubmatch(p)[1] + var names []string + for full := range f.repos { + if o, n, _ := strings.Cut(full, "/"); o == owner { + names = append(names, n) + } + } + sort.Strings(names) + size := f.pageSize + if size == 0 { + size = 50 + } + if l, _ := strconv.Atoi(r.URL.Query().Get("limit")); l > 0 && l < size { + size = l + } + page, _ := strconv.Atoi(r.URL.Query().Get("page")) + if page < 1 { + page = 1 + } + out := []map[string]any{} + for i := (page - 1) * size; i < len(names) && i < page*size; i++ { + out = append(out, map[string]any{"name": names[i], "full_name": owner + "/" + names[i]}) + } + reply(200, out) case p == "/api/v1/user/repos" && r.Method == http.MethodPost: name := fmt.Sprint(in["name"]) if f.repos[admin+"/"+name] != nil { @@ -312,12 +341,3 @@ func result(o Outcome, name string) Result { } return Result{} } - -func has(list []string, want string) bool { - for _, s := range list { - if s == want { - return true - } - } - return false -} diff --git a/modules/gitea/cmd/repositories/first.go b/modules/gitea/cmd/repositories/first.go index b1303299..1ae7365a 100644 --- a/modules/gitea/cmd/repositories/first.go +++ b/modules/gitea/cmd/repositories/first.go @@ -8,7 +8,7 @@ import "strings" const MergeCheck = `#!/bin/sh # mesh-check-toolchain: go # -# This repository's own check, mesh/repo-check (novox/hq ADR 0238 §4), written with its first commit (ADR 0428 §4): +# This repository's own check (novox/hq ADR 0238 §4), run by the checker within mesh/check (ADR 0447), written with its first commit (ADR 0428 §4): # every Go module in the tree is vetted, its unit tests run, and then its integration tests (-tags integration). # A tree with no Go module has nothing to test, and the check says so and passes. set -eu @@ -37,7 +37,7 @@ func FirstCommit(name string, r Repo) map[string]string { } readme := "# " + name + "\n\n" + purpose + "\n\n" + "Made by the mesh from the gitea module's config value `repositories` (novox/hq ADR 0428). Main takes no push: " + - "a change reaches it by a pull request whose `mesh/repo-check` (this repository's `merge-check.sh`) and " + - "`mesh/merge-gate` passed.\n" + "a change reaches it by a pull request whose `mesh/check` passed: the checker's verdict on its head merged onto " + + "main, this repository's `merge-check.sh` among its tests (novox/hq ADR 0447).\n" return map[string]string{"README.md": readme, "merge-check.sh": MergeCheck} } diff --git a/modules/gitea/cmd/repositories/main.go b/modules/gitea/cmd/repositories/main.go index 6a1a4c8e..48b707de 100644 --- a/modules/gitea/cmd/repositories/main.go +++ b/modules/gitea/cmd/repositories/main.go @@ -3,7 +3,8 @@ // again whenever the value changes: it reads the file the mesh renders from that value and, with the setup login, // makes every listed repository that does not exist under that login's own account, protects its main, writes // main's first commit, narrows main's rule and only then moves it into the organisation; one that exists only has -// its main rule put right. A repository the value does not list is never touched. It keeps what it did, per +// its main rule put right, requiring mesh/check in place of the two statuses it replaces (issue 1301). A repository +// the value does not list is touched only to swap those two for mesh/check when its main requires mesh/merge-gate. It keeps what it did, per // repository, in the outcome file, says each change on stdout in one line and never a secret, and exits 1 when a // step was refused, its last stderr lines naming each (outcome.go Report): the node-engine's gate on a run-once // step (ADR 0136) reports it and runs it again on every apply until it passes. diff --git a/modules/gitea/cmd/repositories/override_test.go b/modules/gitea/cmd/repositories/override_test.go index b26f520d..8dc5d7bc 100644 --- a/modules/gitea/cmd/repositories/override_test.go +++ b/modules/gitea/cmd/repositories/override_test.go @@ -18,7 +18,7 @@ func TestNewRepositoryRefusesTheAdministratorsOverride(t *testing.T) { } // 1141/AC2: a narrowed main rule that still lets an administrator override is not right: it is put right, its -// statuses kept, and a second pass changes nothing. +// statuses kept (the two old ones as mesh/check, issue 1301), and a second pass changes nothing. func TestNarrowedRuleLettingTheAdministratorOverrideIsPutRight(t *testing.T) { f := newFake() repo := f.add("novox/mesh-delivery", map[string]map[string]any{mainRule: { @@ -33,10 +33,8 @@ func TestNarrowedRuleLettingTheAdministratorOverrideIsPutRight(t *testing.T) { if rule["block_admin_merge_override"] != true { t.Errorf("the administrator's override is still allowed: %v", rule) } - for _, c := range []string{"mesh/merge-gate", "mesh/repo-check", "other/x"} { - if !has(strs(rule["status_check_contexts"]), c) { - t.Errorf("a status was dropped: %v", rule["status_check_contexts"]) - } + if c := strs(rule["status_check_contexts"]); strings.Join(c, ",") != "other/x,mesh/check" { + t.Errorf("statuses %v, want other/x kept and mesh/check for the two it replaces (issue 1301)", c) } if r := result(o, "mesh-delivery"); r.Did != DidPutRight { t.Errorf("outcome %+v, want put right", r) diff --git a/modules/gitea/cmd/repositories/repositories.go b/modules/gitea/cmd/repositories/repositories.go index 29985eeb..0bc95701 100644 --- a/modules/gitea/cmd/repositories/repositories.go +++ b/modules/gitea/cmd/repositories/repositories.go @@ -26,9 +26,47 @@ const ( MainBranch = "main" ) -// RequiredStatuses are what main requires in every listed repository from its first commit (ADR 0428 §5): the -// repository's own check and the module graph's gate. A status already on the rule is kept beside them. -var RequiredStatuses = []string{"mesh/merge-gate", "mesh/repo-check"} +// CheckStatus is the one status main requires (ADR 0447 §1, issue 1301): the checker's verdict on a pull request's +// head merged onto its trunk's head. It replaces GateStatus and OwnCheckStatus, and is never dropped. +const CheckStatus = "mesh/check" + +// GateStatus and OwnCheckStatus are the two statuses CheckStatus replaces: the controller's merge gate and the +// repository's own merge-check.sh (ADR 0238 §4, ADR 0428 §5 before ADR 0447 §1). +const ( + GateStatus = "mesh/merge-gate" + OwnCheckStatus = "mesh/repo-check" +) + +// wanted is what a main rule requiring have should require: have with GateStatus and OwnCheckStatus replaced by +// CheckStatus, every other status kept in its order (ADR 0447 §1). A rule requiring OwnCheckStatus with neither +// GateStatus nor CheckStatus is judged by its own script alone, as hq is until issue 1296 settles how hq is +// checked (ADR 0503 §5): the checker writes no status there (ADR 0484 §8), so it is left as it is. This reads the +// rule, not a list of names (lead decision L11 of R4e). +func wanted(have []string) []string { + if !has(have, GateStatus) && !has(have, CheckStatus) && has(have, OwnCheckStatus) { + return have + } + out := []string{} + for _, s := range have { + if s != GateStatus && s != OwnCheckStatus && s != CheckStatus { + out = append(out, s) + } + } + return append(out, CheckStatus) +} + +func has(list []string, want string) bool { + for _, s := range list { + if s == want { + return true + } + } + return false +} + +func sameList(a, b []string) bool { + return strings.Join(a, "\x00") == strings.Join(b, "\x00") +} // The steps of one repository, as an outcome names a refused one (ADR 0428 §6). const ( @@ -42,6 +80,10 @@ const ( // StepRuleAfterMove reads main's rule back once the repository is in the organisation and puts it right: the // forge drops from a merge whitelist a user without access, and under the setup account the tools' have none. StepRuleAfterMove = "rule-after-move" + // StepList reads the organisation's repositories, for those the value does not list (issue 1301). + StepList = "list" + // StepSwap swaps the two old statuses for CheckStatus on a repository the value does not list (issue 1301). + StepSwap = "swap" ) // What a pass did with one repository. @@ -157,9 +199,73 @@ func (s *Syncer) Sync(ctx context.Context, v Value) (Outcome, error) { } out.Repositories = append(out.Repositories, r) } + for _, r := range s.switchUnlisted(ctx, v) { + if r.Did == DidRefused { + s.say("refused: %s/%s at the step %s: %s", s.Org, r.Name, r.Step, r.Said) + } else { + s.say("%s: %s/%s (%s in place of %s and %s)", r.Did, s.Org, r.Name, CheckStatus, GateStatus, OwnCheckStatus) + } + out.Repositories = append(out.Repositories, r) + } return out, nil } +// orgPage is how many repositories one read of the organisation's list asks for. +const orgPage = 50 + +// switchUnlisted swaps the two old statuses for CheckStatus on main's rule of every repository of the organisation +// the value does not list whose main requires GateStatus (issue 1301, lead decisions L4 and L11 of R4e): the +// repositories the merge gate judges are switched in the delivery that has the checker write mesh/check (ADR 0447 +// §5), with no list kept by hand. Only the statuses of a rule covering main are sent; nothing else of a rule the +// value does not hold is changed (ADR 0428 §7 for all of it but the statuses the mesh itself requires). A main that +// does not require GateStatus gets no call. It answers only what it did: a repository switched, or refused. +func (s *Syncer) switchUnlisted(ctx context.Context, v Value) []Result { + var names []string + for page := 1; ; page++ { + var repos []struct { + Name string `json:"name"` + } + q := "?limit=" + fmt.Sprint(orgPage) + "&page=" + fmt.Sprint(page) + if err := s.Forge.GetJSON(ctx, "/api/v1/orgs/"+url.PathEscape(s.Org)+"/repos"+q, &repos); err != nil { + return []Result{refused("(the organisation's repositories)", StepList, err)} + } + for _, r := range repos { + if _, listed := v[r.Name]; !listed && r.Name != "" { + names = append(names, r.Name) + } + } + if len(repos) == 0 || page > 1000 { + break + } + } + sort.Strings(names) + var out []Result + for _, n := range names { + rules, err := s.rules(ctx, s.Org, n) + if err != nil { + out = append(out, refused(n, StepRead, err)) + continue + } + var did *Result + for _, r := range rules { + if ok, _ := path.Match(ruleName(r), MainBranch); !ok || !has(required(r), GateStatus) { + continue + } + body := map[string]any{"enable_status_check": true, "status_check_contexts": wanted(required(r))} + if err := s.Forge.SendJSON(ctx, http.MethodPatch, + repoAPI(s.Org, n)+"/branch_protections/"+url.PathEscape(ruleName(r)), body, nil); err != nil { + did = &Result{Name: n, Did: DidRefused, Step: StepSwap, Said: err.Error()} + break + } + did = &Result{Name: n, Did: DidPutRight} + } + if did != nil { + out = append(out, *did) + } + } + return out +} + // waitForForge is cmd/agent-teams' (teams.go waitForForge): the forge starting with the module answers late. func (s *Syncer) waitForForge(ctx context.Context) error { deadline := time.Now().Add(s.WaitFor) @@ -309,7 +415,7 @@ func strList(v any) []string { return out } -// statuses is what a rule requires, with RequiredStatuses added and none dropped. +// statuses is what the rules require together, as wanted makes it. func statuses(rules ...rule) []string { seen := map[string]bool{} var out []string @@ -320,16 +426,19 @@ func statuses(rules ...rule) []string { } } for _, r := range rules { - if r != nil && r["enable_status_check"] == true { - for _, s := range strList(r["status_check_contexts"]) { - add(s) - } + for _, s := range required(r) { + add(s) } } - for _, s := range RequiredStatuses { - add(s) + return wanted(out) +} + +// required is what a rule requires: its statuses when it checks them, else none. +func required(r rule) []string { + if r == nil || r["enable_status_check"] != true { + return nil } - return out + return strList(r["status_check_contexts"]) } // narrowed is main's rule of ADR 0398 §2 (protection.ts mainRuleBody): no push, only the tools' account merges, @@ -344,7 +453,7 @@ func narrowed(r rule, toolsAccount string, also ...rule) map[string]any { } } -// isRight says whether a rule already is narrowed and requires both statuses. +// isRight says whether a rule already is narrowed and requires what wanted makes of it. func isRight(r rule, toolsAccount string) bool { if r["enable_push"] != false || r["enable_push_whitelist"] == true || r["enable_merge_whitelist"] != true { return false @@ -352,26 +461,15 @@ func isRight(r rule, toolsAccount string) bool { if m := strList(r["merge_whitelist_usernames"]); len(m) != 1 || m[0] != toolsAccount { return false } - return r["enable_status_check"] == true && hasAll(strList(r["status_check_contexts"])) && + have := strList(r["status_check_contexts"]) + return r["enable_status_check"] == true && len(have) > 0 && sameList(have, wanted(have)) && r["block_admin_merge_override"] == true } -func hasAll(have []string) bool { - for _, want := range RequiredStatuses { - found := false - for _, h := range have { - found = found || h == want - } - if !found { - return false - } - } - return true -} - -// putRight makes main's rule in an organisation's repository the narrowed one, keeping every status it requires. +// putRight makes main's rule in an organisation's repository the narrowed one, keeping every status it requires but +// the two CheckStatus replaces (ADR 0447 §1). // A repository with no rule named main gets one, carrying what a glob rule covering main asks (client.ts -// ensureMainMerges, issue 948). It never takes a status away and never unprotects (ADR 0428 §5, §7). +// ensureMainMerges, issue 948). It takes away no other status and never unprotects (ADR 0428 §5, §7). func (s *Syncer) putRight(ctx context.Context, owner, name string) (bool, error) { rules, err := s.rules(ctx, owner, name) if err != nil { diff --git a/modules/gitea/cmd/repositories/repositories_test.go b/modules/gitea/cmd/repositories/repositories_test.go index 90592f35..9d551ab2 100644 --- a/modules/gitea/cmd/repositories/repositories_test.go +++ b/modules/gitea/cmd/repositories/repositories_test.go @@ -17,8 +17,8 @@ import ( const mainRule = "main" // 1124/AC2: a listed repository that does not exist is made under the setup account, given main's rule letting that -// login alone push, given its first commit, narrowed to no push and the tools' merge requiring mesh/repo-check (and -// mesh/merge-gate, 1124/AC5), and only then moved into the organisation. Nothing reaches the organisation before the +// login alone push, given its first commit, narrowed to no push and the tools' merge requiring mesh/check (1124/AC5 +// as issue 1301 changes it), and only then moved into the organisation. Nothing reaches the organisation before the // narrowed rule, and nothing else is pushed to main. func TestNewRepositoryIsMadeInOrder(t *testing.T) { f := newFake() @@ -67,8 +67,8 @@ func TestNewRepositoryIsMadeInOrder(t *testing.T) { t.Errorf("main's merge whitelist is not the tools' account alone: %v", rule) } checks := strs(rule["status_check_contexts"]) - if rule["enable_status_check"] != true || !has(checks, "mesh/repo-check") || !has(checks, "mesh/merge-gate") { - t.Errorf("main does not require mesh/repo-check and mesh/merge-gate (1124/AC5): %v", rule) + if rule["enable_status_check"] != true || strings.Join(checks, ",") != CheckStatus { + t.Errorf("main does not require mesh/check alone (1124/AC5, issue 1301): %v", rule) } if r := result(o, "mesh-delivery"); r.Did != DidMade || r.Step != "" { t.Errorf("outcome %+v, want made", r) @@ -87,7 +87,7 @@ func TestNewRepositoryIsMadeInOrder(t *testing.T) { // 1124/AC2: the forge drops from a merge whitelist a user without access to the repository, and under the setup // account the tools' account has none; so once the repository is in the organisation its main rule is read back and -// put right: the tools' account the only one that merges, both statuses required. +// put right: the tools' account the only one that merges, mesh/check required. func TestOrganisationRuleListsTheToolsAfterTheMove(t *testing.T) { f := newFake() s, _ := syncer(t, f) @@ -104,8 +104,8 @@ func TestOrganisationRuleListsTheToolsAfterTheMove(t *testing.T) { t.Errorf("after the move main takes a push: %v", rule) } checks := strs(rule["status_check_contexts"]) - if rule["enable_status_check"] != true || !has(checks, "mesh/repo-check") || !has(checks, "mesh/merge-gate") { - t.Errorf("after the move main does not require both statuses: %v", rule) + if rule["enable_status_check"] != true || strings.Join(checks, ",") != CheckStatus { + t.Errorf("after the move main does not require mesh/check alone: %v", rule) } if r := result(o, "mesh-delivery"); r.Did != DidMade { t.Errorf("outcome %+v, want made", r) @@ -136,7 +136,8 @@ func TestAdminRuleLetsTheSetupLoginAlonePush(t *testing.T) { } // 1124/AC3: a listed repository that exists in the organisation is not made again, only its main rule put right; -// a rule still letting the setup login push is narrowed, its statuses kept (1124/AC5). +// a rule still letting the setup login push is narrowed, its statuses kept but the two mesh/check replaces (1124/AC5, +// issue 1301). func TestExistingOrganisationRepositoryIsOnlyPutRight(t *testing.T) { f := newFake() hq := f.add("novox/hq", map[string]map[string]any{mainRule: { @@ -157,14 +158,11 @@ func TestExistingOrganisationRepositoryIsOnlyPutRight(t *testing.T) { t.Errorf("hq's rule not narrowed: %v", rule) } checks := strs(rule["status_check_contexts"]) - for _, c := range []string{"other/x", "mesh/merge-gate", "mesh/repo-check"} { - if !has(checks, c) { - t.Errorf("hq's rule lacks %s: %v", c, checks) - } + if strings.Join(checks, ",") != "other/x,mesh/check" { + t.Errorf("hq's rule requires %v, want other/x and mesh/check", checks) } made := bare.Rules[mainRule] - if made == nil || made["enable_push"] != false || !has(strs(made["status_check_contexts"]), "mesh/repo-check") || - !has(strs(made["status_check_contexts"]), "mesh/merge-gate") { + if made == nil || made["enable_push"] != false || strings.Join(strs(made["status_check_contexts"]), ",") != CheckStatus { t.Errorf("bare's rule: %v", made) } if hq.Commits != 0 || bare.Commits != 0 { @@ -214,8 +212,8 @@ func TestRepositoryUnderTheSetupAccountIsFinished(t *testing.T) { } } -// 1124/AC5: putting a rule right adds a missing one of mesh/repo-check and mesh/merge-gate and keeps every status -// already there; no pass drops one. +// 1124/AC5 as issue 1301 changes it: putting a rule right adds mesh/check when the rule requires mesh/merge-gate, +// and keeps every other status there; a rule requiring mesh/repo-check alone keeps it (ADR 0503 §5). func TestStatusesAreAddedAndNeverDropped(t *testing.T) { f := newFake() only := f.add("novox/only", map[string]map[string]any{mainRule: { @@ -224,15 +222,15 @@ func TestStatusesAreAddedAndNeverDropped(t *testing.T) { }}, "main") more := f.add("novox/more", map[string]map[string]any{mainRule: { "enable_push": false, "enable_merge_whitelist": true, "merge_whitelist_usernames": []any{tools}, - "enable_status_check": true, "status_check_contexts": []any{"mesh/merge-gate", "mesh/repo-check", "mesh/delivery"}, + "enable_status_check": true, "status_check_contexts": []any{"mesh/delivery", "mesh/check"}, "block_admin_merge_override": true, // right since issue 1141 }}, "main") s, _ := syncer(t, f) o := run(t, s, Value{"only": {}, "more": {}}) - if c := strs(only.Rules[mainRule]["status_check_contexts"]); !has(c, "mesh/merge-gate") || !has(c, "mesh/repo-check") { + if c := strs(only.Rules[mainRule]["status_check_contexts"]); strings.Join(c, ",") != "mesh/repo-check" { t.Errorf("only: %v", c) } - if c := strs(more.Rules[mainRule]["status_check_contexts"]); len(c) != 3 || !has(c, "mesh/delivery") { + if c := strs(more.Rules[mainRule]["status_check_contexts"]); len(c) != 2 || !has(c, "mesh/delivery") { t.Errorf("more lost a status: %v", c) } for _, w := range f.writes() { @@ -314,7 +312,8 @@ func TestRefusedStepStopsThatRepositoryOnly(t *testing.T) { } } -// 1124/AC7: a repository taken out of the value gets no call at all. +// 1124/AC7: a repository taken out of the value is not changed: its rules are read (issue 1301 switches one whose +// main requires mesh/merge-gate), nothing is written, and one still under the setup account is not asked about. func TestRepositoryOutOfTheValueIsNotTouched(t *testing.T) { f := newFake() f.add("novox/old", map[string]map[string]any{mainRule: {"enable_push": true}}, "main") @@ -323,7 +322,7 @@ func TestRepositoryOutOfTheValueIsNotTouched(t *testing.T) { run(t, s, Value{}) run(t, s, Value{"new": {}}) for _, c := range f.all() { - if strings.Contains(c, "old") { + if strings.Contains(c, "older") || (strings.Contains(c, "old") && !strings.HasPrefix(c, "GET ")) { t.Errorf("a repository out of the value was asked: %s", c) } } diff --git a/modules/gitea/module.json b/modules/gitea/module.json index 10ad38ce..fdec53e8 100644 --- a/modules/gitea/module.json +++ b/modules/gitea/module.json @@ -172,7 +172,7 @@ }, "repositories": { "type": "object", - "description": "The repositories of the organisation novox the mesh holds (ADR 0428): each by name, {\"\": {\"private\": true, \"description\": \"...\"}}. One that does not exist is made by the module's setup login under its own account, its main protected and given a first commit (a README naming the description and a merge-check.sh), its rule narrowed to no push and the tools' merge requiring mesh/repo-check and mesh/merge-gate, and only then moved into the organisation; one that exists only has main's rule put right, its statuses kept. A repository taken out of the value is never touched.", + "description": "The repositories of the organisation novox the mesh holds (ADR 0428): each by name, {\"\": {\"private\": true, \"description\": \"...\"}}. One that does not exist is made by the module's setup login under its own account, its main protected and given a first commit (a README naming the description and a merge-check.sh), its rule narrowed to no push and the tools' merge requiring mesh/check, and only then moved into the organisation; one that exists only has main's rule put right, mesh/check in place of mesh/merge-gate and mesh/repo-check and its other statuses kept (a main requiring mesh/repo-check alone keeps it). Every other repository whose main requires mesh/merge-gate has the two swapped for mesh/check; nothing else of a repository out of the value is touched.", "trusted": true, "default": {} }