From fa397389819567824fa2579dc838d63f03ffac8d Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 23 Oct 2026 16:10:09 +0200 Subject: [PATCH] Revert "Swap hq's repo-check-alone rule to mesh/check like every main: nothing writes repo-check once build-agent goes (issue 1296, E1a)" This reverts commit e4ec060ac7db1f7a48bf9d27a3ed865375d4d5cd. --- modules/gitea/cmd/repositories/check_test.go | 31 +++++++++---------- .../gitea/cmd/repositories/repositories.go | 18 ++++++----- .../cmd/repositories/repositories_test.go | 4 +-- 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/modules/gitea/cmd/repositories/check_test.go b/modules/gitea/cmd/repositories/check_test.go index a2342ee3..0a5dd7aa 100644 --- a/modules/gitea/cmd/repositories/check_test.go +++ b/modules/gitea/cmd/repositories/check_test.go @@ -7,7 +7,7 @@ import ( // 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) is swapped too (ADR 0527 §2); every repository of the organisation whose main requires either is +// `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 { @@ -84,27 +84,30 @@ func TestListedRuleRequiringNothingGetsCheck(t *testing.T) { } } -// 1296/E1 (ADR 0527 §2): a main that requires mesh/repo-check alone (hq) is given mesh/check in its place, listed or -// not, since nothing writes mesh/repo-check once build-agent is gone. -func TestOwnCheckAloneIsSwapped(t *testing.T) { +// 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, ",") != CheckStatus { - t.Errorf("%s requires %v, want %s alone", name, got, CheckStatus) + 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 a change", result(o, "hq")) + 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 is not written to. +// 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: { @@ -112,7 +115,7 @@ func TestUnlistedRepositoryRequiringTheGateIsSwitched(t *testing.T) { }}, "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") - hq := f.add("novox/hq", map[string]map[string]any{mainRule: narrowedRule("mesh/repo-check")}, "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) @@ -127,23 +130,19 @@ func TestUnlistedRepositoryRequiringTheGateIsSwitched(t *testing.T) { if got := strs(toolsGo.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { t.Errorf("mesh-tools-go requires %v, want %s", got, CheckStatus) } - if got := strs(hq.Rules[mainRule]["status_check_contexts"]); strings.Join(got, ",") != CheckStatus { - t.Errorf("hq requires %v, want %s", got, CheckStatus) - } want := []string{ - "PATCH /api/v1/repos/novox/hq/branch_protections/main", "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{"hq", "mesh-catalog", "mesh-tools-go"} { + 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", "norule", "elsewhere"} { + 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) } diff --git a/modules/gitea/cmd/repositories/repositories.go b/modules/gitea/cmd/repositories/repositories.go index a76ad993..0bc95701 100644 --- a/modules/gitea/cmd/repositories/repositories.go +++ b/modules/gitea/cmd/repositories/repositories.go @@ -38,10 +38,14 @@ const ( ) // 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 alone, as hq's -// did, is swapped too: the checker judges hq like any repository and nothing writes OwnCheckStatus once the merge -// check's second layer is gone (ADR 0527 §2, issue 1296). This reads the rule, not a list of names (lead decision L11 of R4e). +// 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 { @@ -210,11 +214,11 @@ func (s *Syncer) Sync(ctx context.Context, v Value) (Outcome, error) { 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 or OwnCheckStatus (issue 1301, lead decisions L4 and L11 of R4e): the +// 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; hq's, ADR 0527 §2), with no list kept by hand. Only the statuses of a rule covering main are sent; nothing else of a rule the +// §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 -// requires neither gets no call. It answers only what it did: a repository switched, or refused. +// 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++ { @@ -244,7 +248,7 @@ func (s *Syncer) switchUnlisted(ctx context.Context, v Value) []Result { } var did *Result for _, r := range rules { - if ok, _ := path.Match(ruleName(r), MainBranch); !ok || !(has(required(r), GateStatus) || has(required(r), OwnCheckStatus)) { + 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))} diff --git a/modules/gitea/cmd/repositories/repositories_test.go b/modules/gitea/cmd/repositories/repositories_test.go index 23b5b451..78f17139 100644 --- a/modules/gitea/cmd/repositories/repositories_test.go +++ b/modules/gitea/cmd/repositories/repositories_test.go @@ -212,7 +212,7 @@ func TestRepositoryUnderTheSetupAccountIsFinished(t *testing.T) { } // 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 is given mesh/check (ADR 0527 §2). +// 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: { @@ -226,7 +226,7 @@ func TestStatusesAreAddedAndNeverDropped(t *testing.T) { }}, "main") s, _ := syncer(t, f) o := run(t, s, Value{"only": {}, "more": {}}) - if c := strs(only.Rules[mainRule]["status_check_contexts"]); strings.Join(c, ",") != CheckStatus { + 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) != 2 || !has(c, "mesh/delivery") {