Skip to content

Commit d3ba591

Browse files
authored
Per-path approval and per-group reviewer suggestions (#4918)
## Why Cross-domain PRs aren't handled correctly by the current maintainer-approval workflow. If a PR touches `/cmd/pipelines/` and `/cmd/apps/`, a single approval from either team covers the whole PR. Each domain team should approve their own area independently. The `suggest-reviewers` comment also doesn't surface which ownership areas need review. It just shows a flat list. On top of that, the workflow uses `core.setFailed()` to block PRs missing approval. This shows a red X, which is misleading: "waiting for approval" isn't an error, it's a normal pending state. ## Changes **Before:** One exemption check (`isExempted`) that only worked when the PR author owned all changed files. Cross-domain PRs fell through to requiring a maintainer. `suggest-reviewers` showed a flat list. Missing approval = red X. **Now:** Per-path approval where each ownership group needs at least one approval from its owners. `suggest-reviewers` shows per-group sections for cross-domain PRs. Missing approval = yellow pending dot via Commit Status API. ### `owners.js` Added `getOwnershipGroups(filenames, rules)` that groups files by their last-match-wins OWNERS pattern. Returns `Map<pattern, { owners, files }>`. This is the shared building block used by both the approval check and reviewer suggestions. ### `maintainer-approval.js` Replaced `isExempted` with `checkPerPathApproval`. The new flow: 1. Any maintainer approved? -> `success` (unchanged) 2. PR author is a maintainer and has any approval? -> `success` (new, any second pair of eyes suffices) 3. Group changed files by ownership. Any files matching only `*`? -> `pending` (needs maintainer) 4. For each ownership group, has at least one owner approved? All covered -> `success`. Some uncovered -> `pending` (lists which groups need approval) Team refs (`@org/team`) are resolved via `getMembershipForUserInOrg`. The resolution distinguishes 404 (not a member) from permission errors (logs a warning, falls back to requiring maintainer). Switched from `core.setFailed()` to `createCommitStatus()` with `pending`/`success` states, so "waiting for approval" is a yellow dot instead of a red X. The ruleset should require the `maintainer-approval` commit status context. ### `suggest-reviewers.js` When a PR crosses 2+ ownership groups, shows "Reviewers by ownership area" with per-group sections. Each section lists the matched files, team refs, and individually ranked owners (by git history score). Single-domain PRs keep the existing flat format. The wildcard section (`*` files) only suggests maintainers, since only they can approve those files. ### `OWNERS` - Added `/bundle/`, `/acceptance/bundle/`, and `/acceptance/labs/` rules mirroring their source counterparts - Added section comments for readability - Grouped rules by domain ### `maintainer-approval.yml` - Added `statuses: write` permission (needed for `createCommitStatus`) ## Test plan - [x] 36 unit tests using Node's built-in `node:test` runner (zero dependencies) - 24 tests for `owners.js`: `ownersMatch`, `parseOwnersFile`, `findOwners`, `getMaintainers`, `getOwnershipGroups` - 12 tests for `maintainer-approval.js` with mocked GitHub API: maintainer approval, maintainer-authored PR self-approval, single domain, cross-domain (both covered and partially covered), wildcard files, team member approval, non-team-member rejection, `CHANGES_REQUESTED` exclusion, self-approval exclusion, missing `*` rule error - Run: `node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js` - [ ] Verify on a single-domain PR (only `/cmd/pipelines/` files): pending until pipelines owner approves - [ ] Verify on a cross-domain PR (`/cmd/pipelines/` + `/cmd/apps/`): pending until both groups approved - [ ] Verify on a PR touching only `*` files: pending, message says "needs maintainer" - [ ] Verify maintainer approval short-circuits: success status immediately - [ ] Update branch ruleset to require `maintainer-approval` commit status context ### Edge case coverage (from unit tests) | Scenario | Expected | Tested | |----------|----------|--------| | PR touches only `*` files | Maintainer required, pending | yes | | PR touches one domain only | One domain owner approval suffices | yes | | PR touches two domains | Both groups need approval | yes | | PR touches domain + `*` files | Maintainer required (wildcard forces it) | yes | | Maintainer approves | Always success | yes | | PR author is maintainer + any approval | Success (second pair of eyes) | yes | | Team member approves team-owned path | Success (via Teams API) | yes | | `CHANGES_REQUESTED` review | Does not count as approval | yes | | PR author self-approval | Excluded from approver list | yes |
1 parent 7688bec commit d3ba591

File tree

8 files changed

+949
-52
lines changed

8 files changed

+949
-52
lines changed

.github/OWNERS

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,25 @@
1-
* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum
2-
/cmd/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db
3-
/libs/template/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @lennartkats-db
4-
/acceptance/pipelines/ @jefferycheng1 @kanterov @lennartkats-db
5-
/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db
6-
/cmd/labs/ @alexott @nfx
7-
/cmd/apps/ @databricks/eng-apps-devex
8-
/cmd/workspace/apps/ @databricks/eng-apps-devex
9-
/libs/apps/ @databricks/eng-apps-devex
10-
/acceptance/apps/ @databricks/eng-apps-devex
11-
/experimental/aitools/ @databricks/eng-apps-devex @lennartkats-db
1+
# Maintainers (can approve any PR)
2+
* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum
3+
4+
# 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
9+
10+
# Pipelines
11+
/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db
12+
/acceptance/pipelines/ @jefferycheng1 @kanterov @lennartkats-db
13+
14+
# Labs
15+
/cmd/labs/ @alexott @nfx
16+
/acceptance/labs/ @alexott @nfx
17+
18+
# Apps
19+
/cmd/apps/ @databricks/eng-apps-devex
20+
/cmd/workspace/apps/ @databricks/eng-apps-devex
21+
/libs/apps/ @databricks/eng-apps-devex
22+
/acceptance/apps/ @databricks/eng-apps-devex
23+
24+
# Experimental
25+
/experimental/aitools/ @databricks/eng-apps-devex @lennartkats-db

.github/scripts/owners.js

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,4 +65,28 @@ function getMaintainers(rules) {
6565
return catchAll ? catchAll.owners : [];
6666
}
6767

68-
module.exports = { parseOwnersFile, ownersMatch, findOwners, getMaintainers };
68+
/**
69+
* Group files by their matched OWNERS rule (last-match-wins).
70+
* Returns Map<pattern, { owners: string[], files: string[] }>
71+
*/
72+
function getOwnershipGroups(filenames, rules) {
73+
const groups = new Map();
74+
for (const filepath of filenames) {
75+
let matchedPattern = null;
76+
let matchedOwners = [];
77+
for (const rule of rules) {
78+
if (ownersMatch(rule.pattern, filepath)) {
79+
matchedPattern = rule.pattern;
80+
matchedOwners = rule.owners;
81+
}
82+
}
83+
if (!matchedPattern) continue;
84+
if (!groups.has(matchedPattern)) {
85+
groups.set(matchedPattern, { owners: [...matchedOwners], files: [] });
86+
}
87+
groups.get(matchedPattern).files.push(filepath);
88+
}
89+
return groups;
90+
}
91+
92+
module.exports = { parseOwnersFile, ownersMatch, findOwners, getMaintainers, getOwnershipGroups };

.github/scripts/owners.test.js

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
const { describe, it, before, after } = require("node:test");
2+
const assert = require("node:assert/strict");
3+
const fs = require("fs");
4+
const os = require("os");
5+
const path = require("path");
6+
7+
const {
8+
ownersMatch,
9+
parseOwnersFile,
10+
findOwners,
11+
getMaintainers,
12+
getOwnershipGroups,
13+
} = require("./owners");
14+
15+
// --- ownersMatch ---
16+
17+
describe("ownersMatch", () => {
18+
it("* matches everything", () => {
19+
assert.ok(ownersMatch("*", "any/file/path.go"));
20+
assert.ok(ownersMatch("*", "README.md"));
21+
assert.ok(ownersMatch("*", ""));
22+
});
23+
24+
it("/dir/ prefix matches files under that directory", () => {
25+
assert.ok(ownersMatch("/cmd/pipelines/", "cmd/pipelines/foo.go"));
26+
assert.ok(ownersMatch("/cmd/pipelines/", "cmd/pipelines/sub/bar.go"));
27+
});
28+
29+
it("/dir/ does NOT match files in other directories", () => {
30+
assert.ok(!ownersMatch("/cmd/pipelines/", "cmd/other/foo.go"));
31+
assert.ok(!ownersMatch("/cmd/pipelines/", "cmd/pipeline/foo.go"));
32+
assert.ok(!ownersMatch("/cmd/pipelines/", "bundle/pipelines/foo.go"));
33+
});
34+
35+
it("exact file match", () => {
36+
assert.ok(ownersMatch("/some/file.go", "some/file.go"));
37+
assert.ok(!ownersMatch("/some/file.go", "some/other.go"));
38+
assert.ok(!ownersMatch("/some/file.go", "some/file.go/extra"));
39+
});
40+
41+
it("leading / is stripped for matching", () => {
42+
assert.ok(ownersMatch("/bundle/", "bundle/config.go"));
43+
assert.ok(ownersMatch("/README.md", "README.md"));
44+
});
45+
});
46+
47+
// --- parseOwnersFile ---
48+
49+
describe("parseOwnersFile", () => {
50+
let tmpDir;
51+
let ownersPath;
52+
53+
before(() => {
54+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-test-"));
55+
ownersPath = path.join(tmpDir, "OWNERS");
56+
});
57+
58+
after(() => {
59+
fs.rmSync(tmpDir, { recursive: true });
60+
});
61+
62+
it("parses rules with owners", () => {
63+
fs.writeFileSync(
64+
ownersPath,
65+
[
66+
"* @alice @bob",
67+
"/cmd/pipelines/ @carol",
68+
].join("\n")
69+
);
70+
const rules = parseOwnersFile(ownersPath);
71+
assert.equal(rules.length, 2);
72+
assert.equal(rules[0].pattern, "*");
73+
assert.deepEqual(rules[0].owners, ["alice", "bob"]);
74+
assert.equal(rules[1].pattern, "/cmd/pipelines/");
75+
assert.deepEqual(rules[1].owners, ["carol"]);
76+
});
77+
78+
it("filters out team refs by default", () => {
79+
fs.writeFileSync(
80+
ownersPath,
81+
"/cmd/apps/ @databricks/eng-apps-devex @alice\n"
82+
);
83+
const rules = parseOwnersFile(ownersPath);
84+
assert.equal(rules.length, 1);
85+
assert.deepEqual(rules[0].owners, ["alice"]);
86+
});
87+
88+
it("includes team refs with includeTeams option", () => {
89+
fs.writeFileSync(
90+
ownersPath,
91+
"/cmd/apps/ @databricks/eng-apps-devex @alice\n"
92+
);
93+
const rules = parseOwnersFile(ownersPath, { includeTeams: true });
94+
assert.equal(rules.length, 1);
95+
assert.deepEqual(rules[0].owners, ["databricks/eng-apps-devex", "alice"]);
96+
});
97+
98+
it("skips comments and blank lines", () => {
99+
fs.writeFileSync(
100+
ownersPath,
101+
[
102+
"# This is a comment",
103+
"",
104+
" # indented comment",
105+
"* @alice",
106+
"",
107+
"/cmd/ @bob",
108+
].join("\n")
109+
);
110+
const rules = parseOwnersFile(ownersPath);
111+
assert.equal(rules.length, 2);
112+
});
113+
114+
it("strips @ prefix from owners", () => {
115+
fs.writeFileSync(ownersPath, "* @alice @bob\n");
116+
const rules = parseOwnersFile(ownersPath);
117+
assert.deepEqual(rules[0].owners, ["alice", "bob"]);
118+
});
119+
120+
it("skips lines with only a pattern and no owners", () => {
121+
fs.writeFileSync(ownersPath, "/lonely/\n* @alice\n");
122+
const rules = parseOwnersFile(ownersPath);
123+
assert.equal(rules.length, 1);
124+
assert.equal(rules[0].pattern, "*");
125+
});
126+
});
127+
128+
// --- findOwners ---
129+
130+
describe("findOwners", () => {
131+
const rules = [
132+
{ pattern: "*", owners: ["maintainer1", "maintainer2"] },
133+
{ pattern: "/cmd/pipelines/", owners: ["pipelinesOwner"] },
134+
{ pattern: "/cmd/apps/", owners: ["appsOwner"] },
135+
];
136+
137+
it("last match wins", () => {
138+
const owners = findOwners("cmd/pipelines/foo.go", rules);
139+
assert.deepEqual(owners, ["pipelinesOwner"]);
140+
});
141+
142+
it("file matching only * returns catch-all owners", () => {
143+
const owners = findOwners("README.md", rules);
144+
assert.deepEqual(owners, ["maintainer1", "maintainer2"]);
145+
});
146+
147+
it("file matching specific rule returns that rule's owners", () => {
148+
const owners = findOwners("cmd/apps/main.go", rules);
149+
assert.deepEqual(owners, ["appsOwner"]);
150+
});
151+
152+
it("returns empty array when no rules match", () => {
153+
const noWildcard = [{ pattern: "/cmd/pipelines/", owners: ["owner1"] }];
154+
const owners = findOwners("bundle/config.go", noWildcard);
155+
assert.deepEqual(owners, []);
156+
});
157+
});
158+
159+
// --- getMaintainers ---
160+
161+
describe("getMaintainers", () => {
162+
it("returns owners from * rule", () => {
163+
const rules = [
164+
{ pattern: "*", owners: ["alice", "bob"] },
165+
{ pattern: "/cmd/", owners: ["carol"] },
166+
];
167+
assert.deepEqual(getMaintainers(rules), ["alice", "bob"]);
168+
});
169+
170+
it("returns empty array if no * rule", () => {
171+
const rules = [{ pattern: "/cmd/", owners: ["carol"] }];
172+
assert.deepEqual(getMaintainers(rules), []);
173+
});
174+
});
175+
176+
// --- getOwnershipGroups ---
177+
178+
describe("getOwnershipGroups", () => {
179+
const rules = [
180+
{ pattern: "*", owners: ["maintainer"] },
181+
{ pattern: "/cmd/pipelines/", owners: ["pipelinesOwner"] },
182+
{ pattern: "/cmd/apps/", owners: ["appsOwner"] },
183+
{ pattern: "/bundle/", owners: ["bundleOwner"] },
184+
];
185+
186+
it("single file matching one rule -> one group", () => {
187+
const groups = getOwnershipGroups(["cmd/pipelines/foo.go"], rules);
188+
assert.equal(groups.size, 1);
189+
assert.ok(groups.has("/cmd/pipelines/"));
190+
assert.deepEqual(groups.get("/cmd/pipelines/").owners, ["pipelinesOwner"]);
191+
assert.deepEqual(groups.get("/cmd/pipelines/").files, ["cmd/pipelines/foo.go"]);
192+
});
193+
194+
it("multiple files matching same rule -> grouped together", () => {
195+
const groups = getOwnershipGroups(
196+
["cmd/pipelines/foo.go", "cmd/pipelines/bar.go"],
197+
rules
198+
);
199+
assert.equal(groups.size, 1);
200+
assert.deepEqual(groups.get("/cmd/pipelines/").files, [
201+
"cmd/pipelines/foo.go",
202+
"cmd/pipelines/bar.go",
203+
]);
204+
});
205+
206+
it("files matching different rules -> separate groups", () => {
207+
const groups = getOwnershipGroups(
208+
["cmd/pipelines/foo.go", "cmd/apps/bar.go"],
209+
rules
210+
);
211+
assert.equal(groups.size, 2);
212+
assert.ok(groups.has("/cmd/pipelines/"));
213+
assert.ok(groups.has("/cmd/apps/"));
214+
});
215+
216+
it("file matching only * -> group with * key", () => {
217+
const groups = getOwnershipGroups(["README.md"], rules);
218+
assert.equal(groups.size, 1);
219+
assert.ok(groups.has("*"));
220+
assert.deepEqual(groups.get("*").owners, ["maintainer"]);
221+
assert.deepEqual(groups.get("*").files, ["README.md"]);
222+
});
223+
224+
it("file matching no rule -> skipped", () => {
225+
const noWildcard = [{ pattern: "/cmd/pipelines/", owners: ["owner1"] }];
226+
const groups = getOwnershipGroups(["unrelated/file.go"], noWildcard);
227+
assert.equal(groups.size, 0);
228+
});
229+
230+
it("cross-domain: /cmd/pipelines/ and /cmd/apps/ -> two groups", () => {
231+
const groups = getOwnershipGroups(
232+
[
233+
"cmd/pipelines/a.go",
234+
"cmd/pipelines/b.go",
235+
"cmd/apps/c.go",
236+
],
237+
rules
238+
);
239+
assert.equal(groups.size, 2);
240+
assert.deepEqual(groups.get("/cmd/pipelines/").files, [
241+
"cmd/pipelines/a.go",
242+
"cmd/pipelines/b.go",
243+
]);
244+
assert.deepEqual(groups.get("/cmd/apps/").files, ["cmd/apps/c.go"]);
245+
});
246+
247+
it("mixed: domain files + *-only files -> both groups present", () => {
248+
const groups = getOwnershipGroups(
249+
["cmd/pipelines/a.go", "README.md"],
250+
rules
251+
);
252+
assert.equal(groups.size, 2);
253+
assert.ok(groups.has("/cmd/pipelines/"));
254+
assert.ok(groups.has("*"));
255+
});
256+
});

0 commit comments

Comments
 (0)