Skip to content

Commit 8bc2b1a

Browse files
authored
Remove CODEOWNERS and fix maintainer-approval in merge queue (#4931)
## Why 1. Approval enforcement is now handled by the `maintainer-approval` GitHub Action and the `.github/OWNERS` file. GitHub's built-in CODEOWNERS is no longer needed. 2. The `maintainer-approval` workflow only triggers on `pull_request_target` and `pull_request_review` events. It never runs for `merge_group` events, so the status is never set on the merge queue commit. This blocks the merge queue indefinitely. 3. `maintainer-approval` uses commit statuses (`createCommitStatus`), which are not clickable in the GitHub checks UI. Check runs (`checks.create`) show a details page with logs and output. ## Changes **CODEOWNERS removal**: Removes `.github/CODEOWNERS`. The `.github/OWNERS` file already contains the same ownership rules, and the maintainer-approval workflow reads from OWNERS. **Merge queue fix**: Adds `merge_group` trigger to the `maintainer-approval` workflow with an auto-approve job. PRs are already approved before entering the merge queue, so the job sets a passing check on the merge queue commit. This follows the same pattern as the `Integration Tests` auto-approve in `push.yml`. **Commit status to check run**: Migrates all `createCommitStatus` calls to `checks.create` in both the JS script and the YAML workflow. Permissions updated from `statuses: write` to `checks: write`. The `check` job is also guarded with `github.event_name != 'merge_group'` to prevent it from running (and failing) on merge queue events. ## Test plan - [x] Verified OWNERS file covers all paths from CODEOWNERS - [x] `maintainer-approval` is a required status check on main - [x] All 20 tests in `maintainer-approval.test.js` pass with the check run migration - [ ] Verify merge queue succeeds after merging this change
1 parent 048a854 commit 8bc2b1a

8 files changed

Lines changed: 297 additions & 89 deletions

File tree

.github/CODEOWNERS

Lines changed: 0 additions & 11 deletions
This file was deleted.

.github/OWNERS

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
# Maintainers (can approve any PR)
2-
* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum
2+
* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @renaudhartert-db
33

44
# Bundles
5-
/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db
6-
/cmd/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db
7-
/acceptance/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db
8-
/libs/template/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @lennartkats-db
5+
/bundle/ team:bundle @lennartkats-db
6+
/cmd/bundle/ team:bundle @lennartkats-db
7+
/acceptance/bundle/ team:bundle @lennartkats-db
8+
/libs/template/ team:bundle @lennartkats-db
99

1010
# Pipelines
1111
/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db
@@ -21,5 +21,43 @@
2121
/libs/apps/ @databricks/eng-apps-devex
2222
/acceptance/apps/ @databricks/eng-apps-devex
2323

24+
# Auth
25+
/cmd/auth/ team:platform
26+
/libs/auth/ team:platform
27+
/acceptance/auth/ team:platform
28+
29+
# Filesystem & sync
30+
/cmd/fs/ team:platform
31+
/cmd/sync/ team:platform
32+
/libs/filer/ team:platform
33+
/libs/sync/ team:platform
34+
35+
# Core CLI infrastructure
36+
/cmd/root/ team:platform
37+
/cmd/version/ team:platform
38+
/cmd/completion/ team:platform
39+
/cmd/configure/ team:platform
40+
/cmd/cache/ team:platform
41+
/cmd/api/ team:platform
42+
/cmd/selftest/ team:platform
43+
/cmd/psql/ team:platform
44+
/libs/psql/ team:platform
45+
46+
# Libs (general)
47+
/libs/databrickscfg/ team:platform
48+
/libs/env/ team:platform
49+
/libs/flags/ team:platform
50+
/libs/cmdio/ team:platform
51+
/libs/log/ team:platform
52+
/libs/telemetry/ team:platform
53+
/libs/process/ team:platform
54+
/libs/git/ team:platform
55+
56+
# Integration tests
57+
/integration/ team:platform
58+
59+
# Internal
60+
/internal/ team:platform
61+
2462
# Experimental
2563
/experimental/aitools/ @databricks/eng-apps-devex @lennartkats-db

.github/OWNERTEAMS

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# Team aliases for OWNERS file.
2+
# Use "team:<name>" in OWNERS to reference a team defined here.
3+
# Format: team:<name> @member1 @member2 ...
4+
5+
team:bundle @andrewnester @anton-107 @denik @janniklasrose @pietern @shreyas-goenka
6+
team:platform @simonfaltum @renaudhartert-db @hectorcast-db @parthban-db @tanmay-db @Divyansh-db @tejaskochar-db @mihaimitrea-db @chrisst @rauchy

.github/scripts/owners.js

Lines changed: 59 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,56 @@
11
const fs = require("fs");
2+
const path = require("path");
3+
4+
/**
5+
* Read a file and return non-empty, non-comment lines split by whitespace.
6+
* Returns [] if the file does not exist.
7+
*
8+
* @param {string} filePath
9+
* @returns {string[][]} array of whitespace-split tokens per line
10+
*/
11+
function readDataLines(filePath) {
12+
let content;
13+
try {
14+
content = fs.readFileSync(filePath, "utf-8");
15+
} catch (e) {
16+
if (e.code === "ENOENT") return [];
17+
throw e;
18+
}
19+
const result = [];
20+
for (const raw of content.split("\n")) {
21+
const line = raw.trim();
22+
if (!line || line.startsWith("#")) continue;
23+
const parts = line.split(/\s+/);
24+
if (parts.length >= 2) result.push(parts);
25+
}
26+
return result;
27+
}
28+
29+
/**
30+
* Parse an OWNERTEAMS file into a map of team aliases.
31+
* Format: "team:<name> @member1 @member2 ..."
32+
* Returns Map<string, string[]> where key is "team:<name>" and value is member logins.
33+
*
34+
* @param {string} filePath - absolute path to the OWNERTEAMS file
35+
* @returns {Map<string, string[]>}
36+
*/
37+
function parseOwnerTeams(filePath) {
38+
const teams = new Map();
39+
for (const parts of readDataLines(filePath)) {
40+
if (!parts[0].startsWith("team:")) continue;
41+
const members = parts.slice(1).filter((p) => p.startsWith("@")).map((p) => p.slice(1));
42+
teams.set(parts[0], members);
43+
}
44+
return teams;
45+
}
246

347
/**
448
* Parse an OWNERS file (same format as CODEOWNERS).
549
* Returns array of { pattern, owners } rules.
650
*
51+
* If an OWNERTEAMS file exists alongside the OWNERS file, "team:<name>"
52+
* tokens are expanded to their member lists.
53+
*
754
* By default, team refs (org/team) are filtered out and @ is stripped.
855
* Pass { includeTeams: true } to keep team refs (with @ stripped).
956
*
@@ -13,18 +60,19 @@ const fs = require("fs");
1360
*/
1461
function parseOwnersFile(filePath, opts) {
1562
const includeTeams = opts && opts.includeTeams;
16-
const lines = fs.readFileSync(filePath, "utf-8").split("\n");
63+
const teamsPath = path.join(path.dirname(filePath), "OWNERTEAMS");
64+
const teams = parseOwnerTeams(teamsPath);
1765
const rules = [];
18-
for (const raw of lines) {
19-
const line = raw.trim();
20-
if (!line || line.startsWith("#")) continue;
21-
const parts = line.split(/\s+/);
22-
if (parts.length < 2) continue;
66+
for (const parts of readDataLines(filePath)) {
2367
const pattern = parts[0];
24-
const owners = parts
25-
.slice(1)
26-
.filter((p) => p.startsWith("@") && (includeTeams || !p.includes("/")))
27-
.map((p) => p.slice(1));
68+
const owners = [];
69+
for (const p of parts.slice(1)) {
70+
if (p.startsWith("team:") && teams.has(p)) {
71+
owners.push(...teams.get(p));
72+
} else if (p.startsWith("@") && (includeTeams || !p.includes("/"))) {
73+
owners.push(p.slice(1));
74+
}
75+
}
2876
rules.push({ pattern, owners });
2977
}
3078
return rules;
@@ -89,4 +137,4 @@ function getOwnershipGroups(filenames, rules) {
89137
return groups;
90138
}
91139

92-
module.exports = { parseOwnersFile, ownersMatch, findOwners, getMaintainers, getOwnershipGroups };
140+
module.exports = { parseOwnerTeams, parseOwnersFile, ownersMatch, findOwners, getMaintainers, getOwnershipGroups };

.github/scripts/owners.test.js

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const os = require("os");
55
const path = require("path");
66

77
const {
8+
parseOwnerTeams,
89
ownersMatch,
910
parseOwnersFile,
1011
findOwners,
@@ -125,6 +126,99 @@ describe("parseOwnersFile", () => {
125126
});
126127
});
127128

129+
// --- parseOwnerTeams ---
130+
131+
describe("parseOwnerTeams", () => {
132+
let tmpDir;
133+
134+
before(() => {
135+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "ownerteams-test-"));
136+
});
137+
138+
after(() => {
139+
fs.rmSync(tmpDir, { recursive: true });
140+
});
141+
142+
it("parses team definitions", () => {
143+
const teamsPath = path.join(tmpDir, "OWNERTEAMS");
144+
fs.writeFileSync(teamsPath, "team:platform @alice @bob @carol\n");
145+
const teams = parseOwnerTeams(teamsPath);
146+
assert.equal(teams.size, 1);
147+
assert.deepEqual(teams.get("team:platform"), ["alice", "bob", "carol"]);
148+
});
149+
150+
it("parses multiple teams", () => {
151+
const teamsPath = path.join(tmpDir, "OWNERTEAMS");
152+
fs.writeFileSync(teamsPath, "team:platform @alice @bob\nteam:bundle @carol @dave\n");
153+
const teams = parseOwnerTeams(teamsPath);
154+
assert.equal(teams.size, 2);
155+
assert.deepEqual(teams.get("team:platform"), ["alice", "bob"]);
156+
assert.deepEqual(teams.get("team:bundle"), ["carol", "dave"]);
157+
});
158+
159+
it("skips comments and blank lines", () => {
160+
const teamsPath = path.join(tmpDir, "OWNERTEAMS");
161+
fs.writeFileSync(teamsPath, "# comment\n\nteam:platform @alice\n");
162+
const teams = parseOwnerTeams(teamsPath);
163+
assert.equal(teams.size, 1);
164+
});
165+
166+
it("returns empty map if file does not exist", () => {
167+
const teams = parseOwnerTeams(path.join(tmpDir, "NONEXISTENT"));
168+
assert.equal(teams.size, 0);
169+
});
170+
});
171+
172+
// --- parseOwnersFile with team aliases ---
173+
174+
describe("parseOwnersFile with OWNERTEAMS", () => {
175+
let tmpDir;
176+
let ownersPath;
177+
let teamsPath;
178+
179+
before(() => {
180+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-teams-test-"));
181+
ownersPath = path.join(tmpDir, "OWNERS");
182+
teamsPath = path.join(tmpDir, "OWNERTEAMS");
183+
});
184+
185+
after(() => {
186+
fs.rmSync(tmpDir, { recursive: true });
187+
});
188+
189+
it("expands team aliases to members", () => {
190+
fs.writeFileSync(teamsPath, "team:platform @alice @bob\n");
191+
fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform\n");
192+
const rules = parseOwnersFile(ownersPath);
193+
assert.equal(rules.length, 1);
194+
assert.deepEqual(rules[0].owners, ["alice", "bob"]);
195+
});
196+
197+
it("mixes team aliases with individual owners", () => {
198+
fs.writeFileSync(teamsPath, "team:platform @alice @bob\n");
199+
fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform @carol\n");
200+
const rules = parseOwnersFile(ownersPath);
201+
assert.equal(rules.length, 1);
202+
assert.deepEqual(rules[0].owners, ["alice", "bob", "carol"]);
203+
});
204+
205+
it("unknown team alias is ignored", () => {
206+
fs.writeFileSync(teamsPath, "team:platform @alice\n");
207+
fs.writeFileSync(ownersPath, "/cmd/auth/ team:unknown @bob\n");
208+
const rules = parseOwnersFile(ownersPath);
209+
assert.deepEqual(rules[0].owners, ["bob"]);
210+
});
211+
212+
it("works without OWNERTEAMS file", () => {
213+
const tmpDir2 = fs.mkdtempSync(path.join(os.tmpdir(), "owners-noteams-"));
214+
const ownersPath2 = path.join(tmpDir2, "OWNERS");
215+
fs.writeFileSync(ownersPath2, "* @alice\n");
216+
const rules = parseOwnersFile(ownersPath2);
217+
assert.deepEqual(rules[0].owners, ["alice"]);
218+
fs.rmSync(tmpDir2, { recursive: true });
219+
});
220+
});
221+
128222
// --- findOwners ---
129223

130224
describe("findOwners", () => {

.github/workflows/maintainer-approval.js

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -443,11 +443,11 @@ module.exports = async ({ github, context, core }) => {
443443
const prNumber = context.issue.number;
444444
const authorLogin = pr?.user?.login;
445445
const sha = pr.head.sha;
446-
const statusParams = {
446+
const checkParams = {
447447
owner: context.repo.owner,
448448
repo: context.repo.repo,
449-
sha,
450-
context: STATUS_CONTEXT,
449+
head_sha: sha,
450+
name: STATUS_CONTEXT,
451451
};
452452

453453
const reviews = await github.paginate(github.rest.pulls.listReviews, {
@@ -464,10 +464,11 @@ module.exports = async ({ github, context, core }) => {
464464
if (maintainerApproval) {
465465
const approver = maintainerApproval.user.login;
466466
core.info(`Maintainer approval from @${approver}`);
467-
await github.rest.repos.createCommitStatus({
468-
...statusParams,
469-
state: "success",
470-
description: `Approved by @${approver}`,
467+
await github.rest.checks.create({
468+
...checkParams,
469+
status: "completed",
470+
conclusion: "success",
471+
output: { title: STATUS_CONTEXT, summary: `Approved by @${approver}` },
471472
});
472473
await deleteMarkerComments(github, owner, repo, prNumber);
473474
return;
@@ -481,10 +482,11 @@ module.exports = async ({ github, context, core }) => {
481482
);
482483
if (hasAnyApproval) {
483484
core.info(`Maintainer-authored PR approved by a reviewer.`);
484-
await github.rest.repos.createCommitStatus({
485-
...statusParams,
486-
state: "success",
487-
description: "Approved (maintainer-authored PR)",
485+
await github.rest.checks.create({
486+
...checkParams,
487+
status: "completed",
488+
conclusion: "success",
489+
output: { title: STATUS_CONTEXT, summary: "Approved (maintainer-authored PR)" },
488490
});
489491
await deleteMarkerComments(github, owner, repo, prNumber);
490492
return;
@@ -517,10 +519,11 @@ module.exports = async ({ github, context, core }) => {
517519
// Set commit status. Approved PRs return early (commit status is sufficient).
518520
if (result.allCovered && approverLogins.length > 0) {
519521
core.info("All ownership groups have per-path approval.");
520-
await github.rest.repos.createCommitStatus({
521-
...statusParams,
522-
state: "success",
523-
description: "All ownership groups approved",
522+
await github.rest.checks.create({
523+
...checkParams,
524+
status: "completed",
525+
conclusion: "success",
526+
output: { title: STATUS_CONTEXT, summary: "All ownership groups approved" },
524527
});
525528
await deleteMarkerComments(github, owner, repo, prNumber);
526529
return;
@@ -532,10 +535,10 @@ module.exports = async ({ github, context, core }) => {
532535
`Files need maintainer review: ${fileList}. ` +
533536
`Maintainers: ${maintainers.join(", ")}`;
534537
core.info(msg);
535-
await github.rest.repos.createCommitStatus({
536-
...statusParams,
537-
state: "pending",
538-
description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg,
538+
await github.rest.checks.create({
539+
...checkParams,
540+
status: "in_progress",
541+
output: { title: STATUS_CONTEXT, summary: msg },
539542
});
540543
} else if (result.uncovered && result.uncovered.length > 0) {
541544
const groupList = result.uncovered
@@ -545,18 +548,18 @@ module.exports = async ({ github, context, core }) => {
545548
core.info(
546549
`${msg}. Alternatively, any maintainer can approve: ${maintainers.join(", ")}.`
547550
);
548-
await github.rest.repos.createCommitStatus({
549-
...statusParams,
550-
state: "pending",
551-
description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg,
551+
await github.rest.checks.create({
552+
...checkParams,
553+
status: "in_progress",
554+
output: { title: STATUS_CONTEXT, summary: msg },
552555
});
553556
} else {
554557
const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`;
555558
core.info(msg);
556-
await github.rest.repos.createCommitStatus({
557-
...statusParams,
558-
state: "pending",
559-
description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg,
559+
await github.rest.checks.create({
560+
...checkParams,
561+
status: "in_progress",
562+
output: { title: STATUS_CONTEXT, summary: msg },
560563
});
561564
}
562565

0 commit comments

Comments
 (0)