diff --git a/.github/OWNERS b/.github/OWNERS index fe64467b98..6ac08fddc4 100644 --- a/.github/OWNERS +++ b/.github/OWNERS @@ -1,11 +1,25 @@ -* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum -/cmd/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db -/libs/template/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @lennartkats-db -/acceptance/pipelines/ @jefferycheng1 @kanterov @lennartkats-db -/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db -/cmd/labs/ @alexott @nfx -/cmd/apps/ @databricks/eng-apps-devex -/cmd/workspace/apps/ @databricks/eng-apps-devex -/libs/apps/ @databricks/eng-apps-devex -/acceptance/apps/ @databricks/eng-apps-devex -/experimental/aitools/ @databricks/eng-apps-devex @lennartkats-db +# Maintainers (can approve any PR) +* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum + +# Bundles +/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db +/cmd/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db +/acceptance/bundle/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @lennartkats-db +/libs/template/ @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @lennartkats-db + +# Pipelines +/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db +/acceptance/pipelines/ @jefferycheng1 @kanterov @lennartkats-db + +# Labs +/cmd/labs/ @alexott @nfx +/acceptance/labs/ @alexott @nfx + +# Apps +/cmd/apps/ @databricks/eng-apps-devex +/cmd/workspace/apps/ @databricks/eng-apps-devex +/libs/apps/ @databricks/eng-apps-devex +/acceptance/apps/ @databricks/eng-apps-devex + +# Experimental +/experimental/aitools/ @databricks/eng-apps-devex @lennartkats-db diff --git a/.github/scripts/owners.js b/.github/scripts/owners.js index 55bb6b269f..03ac253a5e 100644 --- a/.github/scripts/owners.js +++ b/.github/scripts/owners.js @@ -65,4 +65,28 @@ function getMaintainers(rules) { return catchAll ? catchAll.owners : []; } -module.exports = { parseOwnersFile, ownersMatch, findOwners, getMaintainers }; +/** + * Group files by their matched OWNERS rule (last-match-wins). + * Returns Map + */ +function getOwnershipGroups(filenames, rules) { + const groups = new Map(); + for (const filepath of filenames) { + let matchedPattern = null; + let matchedOwners = []; + for (const rule of rules) { + if (ownersMatch(rule.pattern, filepath)) { + matchedPattern = rule.pattern; + matchedOwners = rule.owners; + } + } + if (!matchedPattern) continue; + if (!groups.has(matchedPattern)) { + groups.set(matchedPattern, { owners: [...matchedOwners], files: [] }); + } + groups.get(matchedPattern).files.push(filepath); + } + return groups; +} + +module.exports = { parseOwnersFile, ownersMatch, findOwners, getMaintainers, getOwnershipGroups }; diff --git a/.github/scripts/owners.test.js b/.github/scripts/owners.test.js new file mode 100644 index 0000000000..65ca79bba4 --- /dev/null +++ b/.github/scripts/owners.test.js @@ -0,0 +1,256 @@ +const { describe, it, before, after } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("fs"); +const os = require("os"); +const path = require("path"); + +const { + ownersMatch, + parseOwnersFile, + findOwners, + getMaintainers, + getOwnershipGroups, +} = require("./owners"); + +// --- ownersMatch --- + +describe("ownersMatch", () => { + it("* matches everything", () => { + assert.ok(ownersMatch("*", "any/file/path.go")); + assert.ok(ownersMatch("*", "README.md")); + assert.ok(ownersMatch("*", "")); + }); + + it("/dir/ prefix matches files under that directory", () => { + assert.ok(ownersMatch("/cmd/pipelines/", "cmd/pipelines/foo.go")); + assert.ok(ownersMatch("/cmd/pipelines/", "cmd/pipelines/sub/bar.go")); + }); + + it("/dir/ does NOT match files in other directories", () => { + assert.ok(!ownersMatch("/cmd/pipelines/", "cmd/other/foo.go")); + assert.ok(!ownersMatch("/cmd/pipelines/", "cmd/pipeline/foo.go")); + assert.ok(!ownersMatch("/cmd/pipelines/", "bundle/pipelines/foo.go")); + }); + + it("exact file match", () => { + assert.ok(ownersMatch("/some/file.go", "some/file.go")); + assert.ok(!ownersMatch("/some/file.go", "some/other.go")); + assert.ok(!ownersMatch("/some/file.go", "some/file.go/extra")); + }); + + it("leading / is stripped for matching", () => { + assert.ok(ownersMatch("/bundle/", "bundle/config.go")); + assert.ok(ownersMatch("/README.md", "README.md")); + }); +}); + +// --- parseOwnersFile --- + +describe("parseOwnersFile", () => { + let tmpDir; + let ownersPath; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-test-")); + ownersPath = path.join(tmpDir, "OWNERS"); + }); + + after(() => { + fs.rmSync(tmpDir, { recursive: true }); + }); + + it("parses rules with owners", () => { + fs.writeFileSync( + ownersPath, + [ + "* @alice @bob", + "/cmd/pipelines/ @carol", + ].join("\n") + ); + const rules = parseOwnersFile(ownersPath); + assert.equal(rules.length, 2); + assert.equal(rules[0].pattern, "*"); + assert.deepEqual(rules[0].owners, ["alice", "bob"]); + assert.equal(rules[1].pattern, "/cmd/pipelines/"); + assert.deepEqual(rules[1].owners, ["carol"]); + }); + + it("filters out team refs by default", () => { + fs.writeFileSync( + ownersPath, + "/cmd/apps/ @databricks/eng-apps-devex @alice\n" + ); + const rules = parseOwnersFile(ownersPath); + assert.equal(rules.length, 1); + assert.deepEqual(rules[0].owners, ["alice"]); + }); + + it("includes team refs with includeTeams option", () => { + fs.writeFileSync( + ownersPath, + "/cmd/apps/ @databricks/eng-apps-devex @alice\n" + ); + const rules = parseOwnersFile(ownersPath, { includeTeams: true }); + assert.equal(rules.length, 1); + assert.deepEqual(rules[0].owners, ["databricks/eng-apps-devex", "alice"]); + }); + + it("skips comments and blank lines", () => { + fs.writeFileSync( + ownersPath, + [ + "# This is a comment", + "", + " # indented comment", + "* @alice", + "", + "/cmd/ @bob", + ].join("\n") + ); + const rules = parseOwnersFile(ownersPath); + assert.equal(rules.length, 2); + }); + + it("strips @ prefix from owners", () => { + fs.writeFileSync(ownersPath, "* @alice @bob\n"); + const rules = parseOwnersFile(ownersPath); + assert.deepEqual(rules[0].owners, ["alice", "bob"]); + }); + + it("skips lines with only a pattern and no owners", () => { + fs.writeFileSync(ownersPath, "/lonely/\n* @alice\n"); + const rules = parseOwnersFile(ownersPath); + assert.equal(rules.length, 1); + assert.equal(rules[0].pattern, "*"); + }); +}); + +// --- findOwners --- + +describe("findOwners", () => { + const rules = [ + { pattern: "*", owners: ["maintainer1", "maintainer2"] }, + { pattern: "/cmd/pipelines/", owners: ["pipelinesOwner"] }, + { pattern: "/cmd/apps/", owners: ["appsOwner"] }, + ]; + + it("last match wins", () => { + const owners = findOwners("cmd/pipelines/foo.go", rules); + assert.deepEqual(owners, ["pipelinesOwner"]); + }); + + it("file matching only * returns catch-all owners", () => { + const owners = findOwners("README.md", rules); + assert.deepEqual(owners, ["maintainer1", "maintainer2"]); + }); + + it("file matching specific rule returns that rule's owners", () => { + const owners = findOwners("cmd/apps/main.go", rules); + assert.deepEqual(owners, ["appsOwner"]); + }); + + it("returns empty array when no rules match", () => { + const noWildcard = [{ pattern: "/cmd/pipelines/", owners: ["owner1"] }]; + const owners = findOwners("bundle/config.go", noWildcard); + assert.deepEqual(owners, []); + }); +}); + +// --- getMaintainers --- + +describe("getMaintainers", () => { + it("returns owners from * rule", () => { + const rules = [ + { pattern: "*", owners: ["alice", "bob"] }, + { pattern: "/cmd/", owners: ["carol"] }, + ]; + assert.deepEqual(getMaintainers(rules), ["alice", "bob"]); + }); + + it("returns empty array if no * rule", () => { + const rules = [{ pattern: "/cmd/", owners: ["carol"] }]; + assert.deepEqual(getMaintainers(rules), []); + }); +}); + +// --- getOwnershipGroups --- + +describe("getOwnershipGroups", () => { + const rules = [ + { pattern: "*", owners: ["maintainer"] }, + { pattern: "/cmd/pipelines/", owners: ["pipelinesOwner"] }, + { pattern: "/cmd/apps/", owners: ["appsOwner"] }, + { pattern: "/bundle/", owners: ["bundleOwner"] }, + ]; + + it("single file matching one rule -> one group", () => { + const groups = getOwnershipGroups(["cmd/pipelines/foo.go"], rules); + assert.equal(groups.size, 1); + assert.ok(groups.has("/cmd/pipelines/")); + assert.deepEqual(groups.get("/cmd/pipelines/").owners, ["pipelinesOwner"]); + assert.deepEqual(groups.get("/cmd/pipelines/").files, ["cmd/pipelines/foo.go"]); + }); + + it("multiple files matching same rule -> grouped together", () => { + const groups = getOwnershipGroups( + ["cmd/pipelines/foo.go", "cmd/pipelines/bar.go"], + rules + ); + assert.equal(groups.size, 1); + assert.deepEqual(groups.get("/cmd/pipelines/").files, [ + "cmd/pipelines/foo.go", + "cmd/pipelines/bar.go", + ]); + }); + + it("files matching different rules -> separate groups", () => { + const groups = getOwnershipGroups( + ["cmd/pipelines/foo.go", "cmd/apps/bar.go"], + rules + ); + assert.equal(groups.size, 2); + assert.ok(groups.has("/cmd/pipelines/")); + assert.ok(groups.has("/cmd/apps/")); + }); + + it("file matching only * -> group with * key", () => { + const groups = getOwnershipGroups(["README.md"], rules); + assert.equal(groups.size, 1); + assert.ok(groups.has("*")); + assert.deepEqual(groups.get("*").owners, ["maintainer"]); + assert.deepEqual(groups.get("*").files, ["README.md"]); + }); + + it("file matching no rule -> skipped", () => { + const noWildcard = [{ pattern: "/cmd/pipelines/", owners: ["owner1"] }]; + const groups = getOwnershipGroups(["unrelated/file.go"], noWildcard); + assert.equal(groups.size, 0); + }); + + it("cross-domain: /cmd/pipelines/ and /cmd/apps/ -> two groups", () => { + const groups = getOwnershipGroups( + [ + "cmd/pipelines/a.go", + "cmd/pipelines/b.go", + "cmd/apps/c.go", + ], + rules + ); + assert.equal(groups.size, 2); + assert.deepEqual(groups.get("/cmd/pipelines/").files, [ + "cmd/pipelines/a.go", + "cmd/pipelines/b.go", + ]); + assert.deepEqual(groups.get("/cmd/apps/").files, ["cmd/apps/c.go"]); + }); + + it("mixed: domain files + *-only files -> both groups present", () => { + const groups = getOwnershipGroups( + ["cmd/pipelines/a.go", "README.md"], + rules + ); + assert.equal(groups.size, 2); + assert.ok(groups.has("/cmd/pipelines/")); + assert.ok(groups.has("*")); + }); +}); diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index d9b4f6b82a..aedd979961 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -1,34 +1,98 @@ const path = require("path"); const { parseOwnersFile, - findOwners, getMaintainers, + getOwnershipGroups, } = require("../scripts/owners"); -// Check if the PR author is exempted. -// If ALL changed files are owned by non-maintainer owners that include the -// author, the PR can merge with any approval (not necessarily a maintainer). -function isExempted(authorLogin, files, rules, maintainers) { - if (files.length === 0) return false; - const maintainerSet = new Set(maintainers); - for (const { filename } of files) { - const owners = findOwners(filename, rules); - const nonMaintainers = owners.filter((o) => !maintainerSet.has(o)); - if (nonMaintainers.length === 0 || !nonMaintainers.includes(authorLogin)) { +/** + * Check if an approver is a member of a GitHub team. + * Requires org read access on the token; falls back to false if unavailable. + */ +async function isTeamMember(github, org, teamSlug, login, core) { + try { + const { data } = await github.rest.teams.getMembershipForUserInOrg({ + org, + team_slug: teamSlug, + username: login, + }); + return data.state === "active"; + } catch (err) { + if (err.status === 404) { + // User is genuinely not a member of this team. return false; } + // Permission denied or other error. Log it so it's visible. + if (core) { + core.warning( + `Could not verify team membership for ${login} in ${org}/${teamSlug} ` + + `(HTTP ${err.status || "unknown"}). Team-based approval may not work ` + + `without a token with org:read scope.` + ); + } + return false; + } +} + +/** + * Check if any approver matches an owner entry. + * Owner can be a plain login or an org/team ref (containing "/"). + */ +async function ownerHasApproval(owner, approverSet, github, org, core) { + if (owner.includes("/")) { + const teamSlug = owner.split("/")[1]; + for (const approver of approverSet) { + if (await isTeamMember(github, org, teamSlug, approver, core)) { + return true; + } + } + return false; } - return true; + return approverSet.has(owner.toLowerCase()); } +/** + * Per-path approval check. Each ownership group needs at least one + * approval from its owners. Files matching only "*" require a maintainer. + */ +async function checkPerPathApproval(files, rulesWithTeams, approverLogins, github, org, core) { + const groups = getOwnershipGroups(files.map(f => f.filename), rulesWithTeams); + const approverSet = new Set(approverLogins.map(l => l.toLowerCase())); + + if (groups.has("*")) { + return { + allCovered: false, + hasWildcardFiles: true, + wildcardFiles: groups.get("*").files, + }; + } + + const uncovered = []; + for (const [pattern, { owners }] of groups) { + let hasApproval = false; + for (const owner of owners) { + if (await ownerHasApproval(owner, approverSet, github, org, core)) { + hasApproval = true; + break; + } + } + if (!hasApproval) { + uncovered.push({ pattern, owners }); + } + } + return { allCovered: uncovered.length === 0, uncovered }; +} + +const STATUS_CONTEXT = "maintainer-approval"; + module.exports = async ({ github, context, core }) => { const ownersPath = path.join( process.env.GITHUB_WORKSPACE, ".github", "OWNERS" ); - const rules = parseOwnersFile(ownersPath); - const maintainers = getMaintainers(rules); + const rulesWithTeams = parseOwnersFile(ownersPath, { includeTeams: true }); + const maintainers = getMaintainers(rulesWithTeams); if (maintainers.length === 0) { core.setFailed( @@ -37,6 +101,16 @@ module.exports = async ({ github, context, core }) => { return; } + const { pull_request: pr } = context.payload; + const authorLogin = pr?.user?.login; + const sha = pr.head.sha; + const statusParams = { + owner: context.repo.owner, + repo: context.repo.repo, + sha, + context: STATUS_CONTEXT, + }; + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, @@ -54,12 +128,39 @@ module.exports = async ({ github, context, core }) => { state === "APPROVED" && user && maintainers.includes(user.login) ); core.info(`Maintainer approval from @${approver.user.login}`); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "success", + description: `Approved by @${approver.user.login}`, + }); return; } - // Check exemption rules based on file ownership. - const { pull_request: pr } = context.payload; - const authorLogin = pr?.user?.login; + // If the PR author is a maintainer, any approval suffices (second pair of eyes). + if (authorLogin && maintainers.includes(authorLogin)) { + const hasAnyApproval = reviews.some( + ({ state, user }) => + state === "APPROVED" && user && user.login !== authorLogin + ); + if (hasAnyApproval) { + core.info(`Maintainer-authored PR approved by a reviewer.`); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "success", + description: "Approved (maintainer-authored PR)", + }); + return; + } + } + + // Gather approved logins (excluding the PR author, since GitHub prevents + // self-approval, but we filter defensively in case of API edge cases). + const approverLogins = reviews + .filter( + ({ state, user }) => + state === "APPROVED" && user && user.login !== authorLogin + ) + .map(({ user }) => user.login); const files = await github.paginate(github.rest.pulls.listFiles, { owner: context.repo.owner, @@ -67,17 +168,60 @@ module.exports = async ({ github, context, core }) => { pull_number: context.issue.number, }); - if (authorLogin && isExempted(authorLogin, files, rules, maintainers)) { - const hasAnyApproval = reviews.some(({ state }) => state === "APPROVED"); - if (!hasAnyApproval) { - core.setFailed( - "PR from exempted author still needs at least one approval." - ); - } + const result = await checkPerPathApproval( + files, + rulesWithTeams, + approverLogins, + github, + context.repo.owner, + core + ); + + if (result.allCovered && approverLogins.length > 0) { + core.info("All ownership groups have per-path approval."); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "success", + description: "All ownership groups approved", + }); return; } - core.setFailed( - `Requires approval from a maintainer: ${maintainers.join(", ")}.` - ); + if (result.hasWildcardFiles) { + const fileList = result.wildcardFiles.join(", "); + const msg = + `Files need maintainer review: ${fileList}. ` + + `Maintainers: ${maintainers.join(", ")}`; + core.info(msg); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "pending", + description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, + }); + return; + } + + if (result.uncovered && result.uncovered.length > 0) { + const groupList = result.uncovered + .map(({ pattern, owners }) => `${pattern} (needs: ${owners.join(", ")})`) + .join("; "); + const msg = `Needs approval: ${groupList}`; + core.info( + `${msg}. Alternatively, any maintainer can approve: ${maintainers.join(", ")}.` + ); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "pending", + description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, + }); + return; + } + + const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`; + core.info(msg); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "pending", + description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, + }); }; diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js new file mode 100644 index 0000000000..da3f0967ea --- /dev/null +++ b/.github/workflows/maintainer-approval.test.js @@ -0,0 +1,322 @@ +const { describe, it, before, after, beforeEach } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("fs"); +const os = require("os"); +const path = require("path"); + +const runModule = require("./maintainer-approval"); + +// --- Test helpers --- + +function makeTmpOwners(content) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "approval-test-")); + const ghDir = path.join(tmpDir, ".github"); + fs.mkdirSync(ghDir); + fs.writeFileSync(path.join(ghDir, "OWNERS"), content); + return tmpDir; +} + +const OWNERS_CONTENT = [ + "* @maintainer1 @maintainer2", + "/cmd/pipelines/ @jefferycheng1 @kanterov", + "/cmd/apps/ @databricks/eng-apps-devex", + "/bundle/ @bundleowner", +].join("\n"); + +function makeContext({ author = "someuser", sha = "abc123", prNumber = 42 } = {}) { + return { + repo: { owner: "databricks", repo: "cli" }, + issue: { number: prNumber }, + payload: { + pull_request: { + user: { login: author }, + head: { sha }, + }, + }, + }; +} + +function makeCore() { + const log = { info: [], warning: [], failed: [] }; + return { + info: (msg) => log.info.push(msg), + warning: (msg) => log.warning.push(msg), + setFailed: (msg) => log.failed.push(msg), + _log: log, + }; +} + +/** + * Build a mock GitHub API object. + * + * @param {Object} opts + * @param {Array} opts.reviews - PR reviews to return + * @param {Array} opts.files - PR files to return (objects with .filename) + * @param {Object} opts.teamMembers - { teamSlug: [logins] } + */ +function makeGithub({ reviews = [], files = [], teamMembers = {} } = {}) { + const listReviews = Symbol("listReviews"); + const listFiles = Symbol("listFiles"); + const statuses = []; + + const github = { + paginate: async (endpoint, _opts) => { + if (endpoint === listReviews) return reviews; + if (endpoint === listFiles) return files; + return []; + }, + rest: { + pulls: { + listReviews, + listFiles, + }, + repos: { + createCommitStatus: async (params) => { + statuses.push(params); + }, + }, + teams: { + getMembershipForUserInOrg: async ({ team_slug, username }) => { + if (teamMembers[team_slug]?.includes(username)) { + return { data: { state: "active" } }; + } + const err = new Error("Not found"); + err.status = 404; + throw err; + }, + }, + }, + _statuses: statuses, + }; + return github; +} + +// --- Tests --- + +describe("maintainer-approval", () => { + let tmpDir; + let originalWorkspace; + + before(() => { + originalWorkspace = process.env.GITHUB_WORKSPACE; + tmpDir = makeTmpOwners(OWNERS_CONTENT); + process.env.GITHUB_WORKSPACE = tmpDir; + }); + + after(() => { + if (originalWorkspace !== undefined) { + process.env.GITHUB_WORKSPACE = originalWorkspace; + } else { + delete process.env.GITHUB_WORKSPACE; + } + fs.rmSync(tmpDir, { recursive: true }); + }); + + it("maintainer approved -> success", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "success"); + assert.ok(github._statuses[0].description.includes("maintainer1")); + }); + + it("maintainer-authored PR with any approval -> success", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "randomreviewer" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext({ author: "maintainer1" }); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "success"); + assert.ok(github._statuses[0].description.includes("maintainer-authored")); + }); + + it("single domain, owner approved -> success", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + ], + files: [ + { filename: "cmd/pipelines/foo.go" }, + { filename: "cmd/pipelines/bar.go" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "success"); + }); + + it("cross-domain, both approved -> success", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + { state: "APPROVED", user: { login: "bundleowner" } }, + ], + files: [ + { filename: "cmd/pipelines/foo.go" }, + { filename: "bundle/config.go" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "success"); + }); + + it("cross-domain, one missing -> pending", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + ], + files: [ + { filename: "cmd/pipelines/foo.go" }, + { filename: "bundle/config.go" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + assert.ok(github._statuses[0].description.includes("/bundle/")); + }); + + it("wildcard files present -> pending, mentions maintainer", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "randomreviewer" } }, + ], + files: [{ filename: "README.md" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + assert.ok(github._statuses[0].description.includes("maintainer")); + }); + + it("no approvals at all -> pending", async () => { + const github = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + }); + + it("team member approved -> success for team-owned path", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "teamdev1" } }, + ], + files: [{ filename: "cmd/apps/main.go" }], + teamMembers: { "eng-apps-devex": ["teamdev1"] }, + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "success"); + }); + + it("non-team-member approval for team-owned path -> pending", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "outsider" } }, + ], + files: [{ filename: "cmd/apps/main.go" }], + teamMembers: { "eng-apps-devex": [] }, + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + }); + + it("CHANGES_REQUESTED does not count as approval", async () => { + const github = makeGithub({ + reviews: [ + { state: "CHANGES_REQUESTED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + }); + + it("self-approval by PR author is excluded", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext({ author: "jefferycheng1" }); + + await runModule({ github, context, core }); + + assert.equal(github._statuses.length, 1); + assert.equal(github._statuses[0].state, "pending"); + }); + + it("no * rule in OWNERS -> setFailed", async () => { + const noWildcardDir = makeTmpOwners("/cmd/pipelines/ @jefferycheng1\n"); + const oldWorkspace = process.env.GITHUB_WORKSPACE; + process.env.GITHUB_WORKSPACE = noWildcardDir; + + const github = makeGithub({ reviews: [], files: [] }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(core._log.failed.length, 1); + assert.ok(core._log.failed[0].includes("maintainers")); + + process.env.GITHUB_WORKSPACE = oldWorkspace; + fs.rmSync(noWildcardDir, { recursive: true }); + }); +}); diff --git a/.github/workflows/maintainer-approval.yml b/.github/workflows/maintainer-approval.yml index 32fca64cd3..d48086f0dc 100644 --- a/.github/workflows/maintainer-approval.yml +++ b/.github/workflows/maintainer-approval.yml @@ -15,8 +15,12 @@ jobs: group: databricks-deco-testing-runner-group labels: ubuntu-latest-deco timeout-minutes: 5 + # Note: team membership resolution in per-path approval requires a token + # with org:read scope. The default GITHUB_TOKEN may lack this, causing + # team-based ownership checks to fall back to maintainer approval. permissions: pull-requests: read + statuses: write steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: diff --git a/.github/workflows/suggest-reviewers.js b/.github/workflows/suggest-reviewers.js index 4d38a1be23..efac10a335 100644 --- a/.github/workflows/suggest-reviewers.js +++ b/.github/workflows/suggest-reviewers.js @@ -3,6 +3,8 @@ const { execSync } = require("child_process"); const { parseOwnersFile, findOwners, + getMaintainers, + getOwnershipGroups, } = require("../scripts/owners"); const MENTION_REVIEWERS = true; @@ -278,6 +280,95 @@ function buildComment( return lines.join("\n") + "\n"; } +function buildPerGroupComment( + groups, + scores, + dirScores, + maintainers, + prAuthor, + totalFiles +) { + const authorLower = prAuthor.toLowerCase(); + const lines = [MARKER, "## Reviewers by ownership area", ""]; + lines.push( + "This PR crosses multiple ownership areas. Each area needs at least one", + "approval from its owners (or any maintainer can approve everything).", + "" + ); + + for (const [pattern, { owners, files }] of groups) { + if (pattern === "*") continue; + + lines.push(`### \`${pattern}\``); + lines.push(`Files: ${files.map((f) => `\`${f}\``).join(", ")}`); + + const teams = owners.filter((o) => o.includes("/")); + const individuals = owners.filter( + (o) => !o.includes("/") && o.toLowerCase() !== authorLower + ); + + if (teams.length > 0) { + lines.push(`Teams: ${teams.map((t) => `@${t}`).join(", ")}`); + } + + if (individuals.length > 0) { + const scored = individuals + .map((o) => [o, scores[o] || 0]) + .sort((a, b) => b[1] - a[1]); + + if (scored[0][1] > 0) { + const mention = MENTION_REVIEWERS ? `@${scored[0][0]}` : scored[0][0]; + lines.push(`Suggested: ${mention}`); + const rest = scored.slice(1).map(([o]) => o); + if (rest.length > 0) { + lines.push(`Also eligible: ${fmtEligible(rest)}`); + } + } else { + lines.push(`Eligible: ${fmtEligible(individuals)}`); + } + } + lines.push(""); + } + + const starGroup = groups.get("*"); + if (starGroup) { + lines.push("### General files (require maintainer)"); + lines.push(`Files: ${starGroup.files.map((f) => `\`${f}\``).join(", ")}`); + + // Only suggest maintainers for wildcard files, since only they can approve. + const maintainerSet = new Set(maintainers.map((m) => m.toLowerCase())); + const maintainerScores = Object.entries(scores) + .filter( + ([login]) => + login.toLowerCase() !== authorLower && + maintainerSet.has(login.toLowerCase()) + ) + .sort((a, b) => b[1] - a[1]); + + if (maintainerScores.length > 0 && maintainerScores[0][1] > 0) { + const [login] = maintainerScores[0]; + const dirs = topDirs(dirScores[login]); + lines.push("Based on git history:"); + lines.push(fmtReviewer(login, dirs)); + } else { + lines.push(`Pick a maintainer from ${OWNERS_LINK}.`); + } + lines.push(""); + } + + const maintainerList = maintainers + .filter((m) => m.toLowerCase() !== authorLower) + .map((m) => (MENTION_REVIEWERS ? `@${m}` : m)) + .join(", "); + + lines.push( + `Any maintainer (${maintainerList}) can approve all areas.`, + `See ${OWNERS_LINK} for ownership rules.` + ); + + return lines.join("\n") + "\n"; +} + async function findExistingComment(github, owner, repo, prNumber) { const comments = await github.paginate(github.rest.issues.listComments, { owner, @@ -318,20 +409,36 @@ module.exports = async ({ github, context, core }) => { const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); const eligible = parseOwnersForFiles(files, ownersPath); - let roundRobin = null; - if (selectReviewers(sortedScores).length === 0 && eligible.length > 0) { - roundRobin = await selectRoundRobin(github, owner, repo, eligible, prAuthor); - } + const rulesWithTeams = parseOwnersFile(ownersPath, { includeTeams: true }); + const groups = getOwnershipGroups(files, rulesWithTeams); + const maintainers = getMaintainers(rulesWithTeams); - const comment = buildComment( - sortedScores, - dirScores, - files.length, - scoredCount, - eligible, - prAuthor, - roundRobin - ); + let comment; + if (groups.size >= 2) { + comment = buildPerGroupComment( + groups, + scores, + dirScores, + maintainers, + prAuthor, + files.length + ); + } else { + let roundRobin = null; + if (selectReviewers(sortedScores).length === 0 && eligible.length > 0) { + roundRobin = await selectRoundRobin(github, owner, repo, eligible, prAuthor); + } + + comment = buildComment( + sortedScores, + dirScores, + files.length, + scoredCount, + eligible, + prAuthor, + roundRobin + ); + } core.info(comment); diff --git a/.github/workflows/test-owners-scripts.yml b/.github/workflows/test-owners-scripts.yml new file mode 100644 index 0000000000..bc09ed4347 --- /dev/null +++ b/.github/workflows/test-owners-scripts.yml @@ -0,0 +1,26 @@ +name: Test OWNERS scripts + +on: + pull_request: + paths: + - '.github/scripts/**' + - '.github/workflows/maintainer-approval.js' + - '.github/workflows/maintainer-approval.test.js' + - '.github/workflows/suggest-reviewers.js' + - '.github/OWNERS' + +jobs: + test: + runs-on: + group: databricks-deco-testing-runner-group + labels: ubuntu-latest-deco + timeout-minutes: 5 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: | + .github/scripts + .github/workflows + .github/OWNERS + - name: Run OWNERS script tests + run: node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js