krz/gitbay

require_checks waits only on CI that would report !391

merged merged by cmc on 2026-09-13 06:57 UTC · krz/gitbay:fix/checks-gate-no-jobs into main

5 files changed, +228 −5

Layout: unified · split

.gitbay/wiki/API.org +6 −2
@@ -109,8 +109,12 @@ One row per (commit, context): re-reporting updates in place. States:
109109worst of them. Statuses appear on commit pages, MR pages, and
110110=mr show=, each with =updated_at=; a =ci/<job>= status also carries
111111=duration=, read from the build behind it, once that build has
112finished. With =repo settings require-checks <repo> on=, merging
113requires the MR head to carry statuses and all of them green. Each
112finished. With =repo settings require-checks <repo> on=, every status
113on the MR head must be green, and a head something was going to report
114on must carry some: a =.gitbay/ci.yml= with a job a push runs, or a
115repository that has recorded a status before, which is what reporting
116from outside through =status set= looks like. A repository where
117nothing has ever reported merges. Each
114118report also emits a =status= event to webhooks.
115119
116120* Webhooks
e2e/ci_nojobs_test.go added +50
@@ -0,0 +1,50 @@
1package e2e
2
3import (
4 "os"
5 "path/filepath"
6 "testing"
7)
8
9// require_checks on a repository with no CI configuration used to refuse
10// every merge with "none were reported", and the only way out was turning
11// the setting off. Nothing was ever going to report, so the gate has
12// nothing to wait for.
13func TestRequireChecksWithoutCIConfigMerges(t *testing.T) {
14 inst := startInstance(t)
15 aliceKey := inst.newKey(t, "alice")
16 inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub",
17 "--email", "alice@example.test", "--verified")
18
19 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/app"); code != 0 {
20 t.Fatalf("repo create: %s", errOut)
21 }
22 if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "settings", "require-checks", "alice/app", "on"); code != 0 {
23 t.Fatalf("require-checks on: %s", errOut)
24 }
25
26 work := t.TempDir()
27 env := inst.gitEnv(aliceKey)
28 mustGit(t, work, env, "clone", inst.sshURL("alice/app"), "w")
29 dir := filepath.Join(work, "w")
30 os.WriteFile(filepath.Join(dir, "README"), []byte("x\n"), 0o644)
31 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
32 mustGit(t, dir, env, "add", ".")
33 mustGit(t, dir, env, "commit", "-q", "-m", "base")
34 mustGit(t, dir, env, "push", "-q", "origin", "main")
35
36 mustGit(t, dir, env, "checkout", "-q", "-b", "feature")
37 os.WriteFile(filepath.Join(dir, "README"), []byte("y\n"), 0o644)
38 mustGit(t, dir, env, "add", ".")
39 mustGit(t, dir, env, "commit", "-q", "-m", "change")
40 mustGit(t, dir, env, "push", "-q", "origin", "feature")
41
42 if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/app",
43 "--source", "feature", "--target", "main", "--title", "change"); code != 0 {
44 t.Fatalf("mr create: %s", errOut)
45 }
46 if out, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/app", "1",
47 "--strategy", "merge"); code != 0 {
48 t.Fatalf("mr merge refused under require_checks with no CI: %s %s", out, errOut)
49 }
50}
internal/control/checksgate_test.go added +120
@@ -0,0 +1,120 @@
1package control
2
3import (
4 "os"
5 "path/filepath"
6 "strings"
7 "testing"
8
9 "gitbay.org/gitbay/internal/store"
10)
11
12// gatesForHead builds a repository with require_checks on, a bare dir
13// holding the given .gitbay/ci.yml (empty string for none), and one MR
14// whose head carries no statuses at all. seed records a status on the
15// base commit, standing for a repository that reports from outside.
16func gatesForHead(t *testing.T, ciYML string) GatesOut {
17 return gatesForHeadSeeded(t, ciYML, false)
18}
19
20func gatesForHeadSeeded(t *testing.T, ciYML string, seed bool) GatesOut {
21 t.Helper()
22 st, repo, uid := newQueueTestRepo(t)
23 if _, err := st.UpdateRepoSettings(repo.ID, func(set *store.RepoSettings) { set.RequireChecks = true }); err != nil {
24 t.Fatal(err)
25 }
26 repo, err := st.RepoByID(repo.ID)
27 if err != nil {
28 t.Fatal(err)
29 }
30
31 git := gitRunner(t)
32 root := t.TempDir()
33 src := filepath.Join(root, "src")
34 os.MkdirAll(src, 0o755)
35 git(root, "init", "-q", "-b", "main", "src")
36 os.WriteFile(filepath.Join(src, "README"), []byte("x\n"), 0o644)
37 git(src, "add", ".")
38 git(src, "commit", "-q", "-m", "base")
39 targetSHA := strings.TrimSpace(git(src, "rev-parse", "HEAD"))
40 git(src, "checkout", "-q", "-b", "feature")
41 if ciYML != "" {
42 os.MkdirAll(filepath.Join(src, ".gitbay"), 0o755)
43 os.WriteFile(filepath.Join(src, ".gitbay", "ci.yml"), []byte(ciYML), 0o644)
44 }
45 os.WriteFile(filepath.Join(src, "README"), []byte("y\n"), 0o644)
46 git(src, "add", ".")
47 git(src, "commit", "-q", "-m", "change")
48 headSHA := strings.TrimSpace(git(src, "rev-parse", "HEAD"))
49
50 dir := RepoDir(root, repo.OwnerName, repo.Name)
51 os.MkdirAll(filepath.Dir(dir), 0o755)
52 git(root, "clone", "-q", "--bare", src, dir)
53
54 if _, err := st.CreateMR(repo.ID, uid, repo.ID, "feature", "main", "t", "", headSHA, "md", false); err != nil {
55 t.Fatal(err)
56 }
57 mr, err := st.MRByNumber(repo.ID, 1)
58 if err != nil {
59 t.Fatal(err)
60 }
61
62 if seed {
63 if err := st.SetCommitStatus(repo.ID, targetSHA, "lint", "success", "", "", uid); err != nil {
64 t.Fatal(err)
65 }
66 }
67
68 g, err := MergeGates(st, repo, mr, dir, targetSHA, headSHA)
69 if err != nil {
70 t.Fatal(err)
71 }
72 return g
73}
74
75func checksUnmet(g GatesOut) bool {
76 for _, u := range g.Unmet {
77 if strings.Contains(u, "green checks") {
78 return true
79 }
80 }
81 return false
82}
83
84// A repository with require_checks on and no CI configuration at its head
85// can never report a status, so the gate has nothing to wait for and the
86// merge must go through rather than be refused forever.
87func TestRequireChecksPassesWithNoCIConfig(t *testing.T) {
88 if g := gatesForHead(t, ""); checksUnmet(g) {
89 t.Fatalf("refused a head with no CI configuration: %v", g.Unmet)
90 }
91}
92
93// Jobs that only run on a schedule or on tags never report on a merge
94// request head either.
95func TestRequireChecksPassesWithNoPushJobs(t *testing.T) {
96 cfg := "jobs:\n nightly:\n schedule: \"0 3 * * *\"\n steps:\n - echo hi\n" +
97 " release:\n tags: \"v*\"\n steps:\n - echo hi\n"
98 if g := gatesForHead(t, cfg); checksUnmet(g) {
99 t.Fatalf("refused a head whose jobs never run on a push: %v", g.Unmet)
100 }
101}
102
103// A push job at the head should have reported something. Silence there
104// means CI did not run, which is what the gate is for.
105func TestRequireChecksRefusesSilentPushJob(t *testing.T) {
106 cfg := "jobs:\n unit:\n steps:\n - echo hi\n"
107 if g := gatesForHead(t, cfg); !checksUnmet(g) {
108 t.Fatalf("allowed a head whose push job reported nothing: %v", g.Unmet)
109 }
110}
111
112// A repository whose checks come from outside — `status set`, no
113// .gitbay/ci.yml — looks like one with no CI at all. Having reported
114// before is what says a report was coming, so a silent head there is
115// still refused.
116func TestRequireChecksRefusesSilentHeadInReportingRepo(t *testing.T) {
117 if g := gatesForHeadSeeded(t, "", true); !checksUnmet(g) {
118 t.Fatalf("allowed a silent head in a repository that reports statuses: %v", g.Unmet)
119 }
120}
internal/control/mr.go +42 −3
@@ -9,6 +9,7 @@ import (
99 "strings"
1010 "time"
1111
12 "gitbay.org/gitbay/internal/ci"
1213 "gitbay.org/gitbay/internal/gitutil"
1314 "gitbay.org/gitbay/internal/policy"
1415 "gitbay.org/gitbay/internal/protocol"
@@ -1243,6 +1244,42 @@ func (c *Ctx) reviewGates(repo store.Repo, mr store.MR, dir, targetSHA, headSHA
12431244 return -1
12441245}
12451246
1247// checksExpected reports whether anything was going to report a status
1248// on this head. A repository with no CI configuration and no history of
1249// statuses can never satisfy require_checks, and refusing its merges
1250// leaves no remedy but turning the setting off. Two things say a report
1251// was coming: a .gitbay/ci.yml at the head with a job a push runs, and a
1252// status having ever been recorded in the repository, which is how a
1253// repository reporting from outside through `status set` looks.
1254func checksExpected(st *store.Store, repoID int64, dir, headSHA string) bool {
1255 if seen, err := st.RepoHasStatuses(repoID); err != nil || seen {
1256 return true
1257 }
1258 return headRunsJobs(dir, headSHA)
1259}
1260
1261// headRunsJobs reports whether a push of this head would have queued or
1262// skipped a job, and so left it a status. A configuration that will not
1263// parse counts as running jobs: the push recorded a ci/config failure
1264// for it, so the head is not silent and this is not the branch that
1265// decides.
1266func headRunsJobs(dir, headSHA string) bool {
1267 raw, err := gitutil.ReadBlob(dir, headSHA, ci.ConfigPath, 1<<16)
1268 if err != nil {
1269 return false
1270 }
1271 jobs, err := ci.Parse(raw)
1272 if err != nil {
1273 return true
1274 }
1275 for _, j := range jobs {
1276 if j.Tags == "" && j.Schedule == "" {
1277 return true
1278 }
1279 }
1280 return false
1281}
1282
12461283// MergeGates computes where a merge request stands against its
12471284// repository's gates: draft, require_checks, require_approvals (fresh,
12481285// non-author, latest review per reviewer from someone who can write; a
@@ -1260,8 +1297,8 @@ func MergeGates(st *store.Store, repo store.Repo, mr store.MR, dir, targetSHA, h
12601297 g.Unmet = append(g.Unmet, fmt.Sprintf("!%d is a draft; `gitbay mr ready %s %d` first", mr.Number, repo.Path(), mr.Number))
12611298 }
12621299
1263 // Checks: with require_checks, the head must carry statuses and every
1264 // one of them must be green.
1300 // Checks: with require_checks, every status the head carries must be
1301 // green, and a head something was going to report on must carry some.
12651302 statuses, err := st.ListCommitStatuses(repo.ID, headSHA)
12661303 if err != nil {
12671304 return g, err
@@ -1271,7 +1308,9 @@ func MergeGates(st *store.Store, repo store.Repo, mr store.MR, dir, targetSHA, h
12711308 switch g.Checks {
12721309 case "success":
12731310 case "":
1274 g.Unmet = append(g.Unmet, fmt.Sprintf("%s requires green checks and none were reported on %.10s", repo.Path(), headSHA))
1311 if checksExpected(st, repo.ID, dir, headSHA) {
1312 g.Unmet = append(g.Unmet, fmt.Sprintf("%s requires green checks and none were reported on %.10s", repo.Path(), headSHA))
1313 }
12751314 default:
12761315 var bad []string
12771316 for _, st := range statuses {
internal/store/statuses.go +10
@@ -53,6 +53,16 @@ func (s *Store) ListCommitStatuses(repoID int64, sha string) ([]CommitStatus, er
5353 return out, rows.Err()
5454}
5555
56// RepoHasStatuses reports whether anything has ever reported a status in
57// this repository. It is how require_checks tells a repository whose
58// checks come from outside — `status set`, with no .gitbay/ci.yml — from
59// one that has no checks at all.
60func (s *Store) RepoHasStatuses(repoID int64) (bool, error) {
61 var n int
62 err := s.DB.QueryRow(`SELECT EXISTS(SELECT 1 FROM commit_statuses WHERE repo_id = ?)`, repoID).Scan(&n)
63 return n == 1, err
64}
65
5666// CombinedStatus reduces per-context states to one: error/failure dominate,
5767// then pending, then success; "" when no statuses exist.
5868// CombinedStatusFor returns the combined state for each of several commits