diff --git a/.agents/skills/onboard-team-area/SKILL.md b/.agents/skills/onboard-team-area/SKILL.md index c7bc9165147..e24020169d4 100644 --- a/.agents/skills/onboard-team-area/SKILL.md +++ b/.agents/skills/onboard-team-area/SKILL.md @@ -1,31 +1,27 @@ --- name: onboard-team-area -description: "Onboard a new team or reserve a new code area in the Databricks CLI: add the team to .github/OWNERTEAMS, reserve experimental// and acceptance/experimental//, and map both paths in .github/OWNERS so the maintainer-approval gate routes the area's PRs to the team. Use when the user says 'onboard a team', 'add an OWNERS team', 'reserve an experimental area', 'add a new team to the CLI', or wants a new owned directory wired into review." +description: "Use when onboarding a team, reserving an experimental area, or assigning code ownership in the Databricks CLI through native GitHub CODEOWNERS." user-invocable: true allowed-tools: Read, Edit, Write, Bash, Glob, Grep, AskUserQuestion --- # Onboard a team / area into the CLI -How ownership works here: `.github/OWNERS` is CODEOWNERS-style and last-match-wins. `.github/OWNERTEAMS` maps `team:` to an explicit `@member` list and is the source of truth, because the CI token cannot resolve GitHub org-team membership. The `maintainer-approval` workflow is a required check that blocks merge until every owned group a PR touches has at least one approval from one of its owners. +How ownership works here: `.github/CODEOWNERS` uses GitHub's native code-owner reviews. The last matching rule wins, and any owner on that line can approve. Include `@databricks/eng-deco-cli` on every rule so maintainers can approve changes across all areas. Team membership is maintained in GitHub. -Worked example: PR #5605 ("Add ai-training OWNERS team and reserve experimental/air") is exactly the steps below: +1 line in OWNERTEAMS, two `.gitkeep` files, +2 lines in OWNERS. +Enforcement requires "Require review from Code Owners" in the target branch's ruleset. Adding a CODEOWNERS file alone requests reviews but does not require them. ## Inputs (ask if missing) -- Team alias, e.g. `ai-training`, and the `@member` list. +- GitHub team slug, e.g. `eng-ai-custom-training`. - Area/dir name, e.g. `air`. - Experimental or stable? This decides where code lands (see the last section). -## Step 1 — Add the team to `.github/OWNERTEAMS` +## Step 1 — Identify the native GitHub team -Append one line, keeping the existing column alignment: +Use the team as `@databricks/`. GitHub requires the team to be visible and have explicit write access to the repository for its approvals to count. -``` -team: @member1 @member2 ... -``` - -If the team has a GitHub team page, add its URL to the header comment block. Skip the URL if the team page does not exist yet; the validator only warns about a missing URL, it does not block. +Use the team slug supplied by the user when they are arranging team creation separately. Verify its access before enabling enforcement. ## Step 2 — Reserve the directories @@ -36,29 +32,28 @@ experimental//.gitkeep acceptance/experimental//.gitkeep ``` -## Step 3 — Map the paths in `.github/OWNERS` +## Step 3 — Map the paths in `.github/CODEOWNERS` -Add rules under an `# ` comment. Because last-match-wins, specific rules go after the `*` maintainer catch-all: +Add rules under an `# ` comment. Specific rules go after the `*` maintainer catch-all and must repeat the maintainer team: ``` -/experimental// team: -/acceptance/experimental// team: +/experimental// @databricks/eng-deco-cli @databricks/ +/acceptance/experimental// @databricks/eng-deco-cli @databricks/ ``` ## Step 4 — Validate and open the PR ```bash -# OWNERS parser + approval-logic tests -node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js -# OWNERS/OWNERTEAMS consistency: undefined teams, zero-owner rules, missing paths -node .github/scripts/owners.js validate +git diff --check # Repo quick checks (no Go/Python/YAML changed, so the formatters have nothing to do) ./task checks ``` +Check that the new paths exist and every new rule includes the maintainer team. Once the branch is pushed, inspect GitHub's CODEOWNERS diagnostics for invalid entries or team permissions. + No `.nextchanges/` entry; this is ownership/config only. Write the PR using the `.github/PULL_REQUEST_TEMPLATE.md` sections (Why / Changes / Tests). ## Experimental vs stable, and graduation - **Experimental** — code under `experimental//`, tests under `acceptance/experimental//`. Register it under the hidden parent in `cmd/experimental/experimental.go`, or top-level in `cmd/cmd.go` with `Hidden: true` (as `ssh` does). Experimental commands still ship enabled in every release; `Hidden` only removes them from `--help`, it does not gate or compile them out. No `.nextchanges/` entries while experimental. To hand a build to testers, push a `bugbash-` branch (auto-builds a snapshot) and share the `internal/bugbash/exec.sh` one-liner. -- **Graduating to stable** — `git mv` the feature-complete commands to `cmd//` + `libs//`, register them top-level in `cmd/cmd.go`, keep the old `experimental` paths as deprecated cobra aliases (`sub.Hidden = true`, `sub.Deprecated = '...'`), add OWNERS rules for the new stable paths, and add a `.nextchanges/` entry. See `experimental/aitools` graduating to top-level `aitools` (PR #4917) as the worked example. +- **Graduating to stable** — `git mv` the feature-complete commands to `cmd//` + `libs//`, register them top-level in `cmd/cmd.go`, keep the old `experimental` paths as deprecated cobra aliases (`sub.Hidden = true`, `sub.Deprecated = '...'`), add CODEOWNERS rules for the new stable paths, and add a `.nextchanges/` entry. See `experimental/aitools` graduating to top-level `aitools` (PR #4917) as the worked example. diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000000..a6c5b3e952f --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,84 @@ +# Maintainers can approve any PR. Keep them on every rule because GitHub uses +# only the last matching rule, and any owner on that line can approve. +* @databricks/eng-deco-cli + +# Bundles +/bundle/ @databricks/eng-deco-cli @databricks/eng-deco-dabs +/cmd/bundle/ @databricks/eng-deco-cli @databricks/eng-deco-dabs +/acceptance/bundle/ @databricks/eng-deco-cli @databricks/eng-deco-dabs +/libs/template/ @databricks/eng-deco-cli @databricks/eng-deco-dabs + +# Pipelines +/cmd/pipelines/ @databricks/eng-deco-cli @jefferycheng1 @kanterov @lennartkats-db +/acceptance/pipelines/ @databricks/eng-deco-cli @jefferycheng1 @kanterov @lennartkats-db + +# Labs +/cmd/labs/ @databricks/eng-deco-cli @alexott @asnare + +# Local environments / DB Connect +/libs/localenv/ @databricks/eng-deco-cli @databricks/eng-deco-ax +/cmd/environments/ @databricks/eng-deco-cli @databricks/eng-deco-ax +/acceptance/localenv/ @databricks/eng-deco-cli @databricks/eng-deco-ax + +# Apps +/cmd/apps/ @databricks/eng-deco-cli @databricks/eng-apps-devex +/cmd/workspace/apps/ @databricks/eng-deco-cli @databricks/eng-apps-devex +/libs/apps/ @databricks/eng-deco-cli @databricks/eng-apps-devex +/acceptance/apps/ @databricks/eng-deco-cli @databricks/eng-apps-devex + +# Sandbox +/cmd/sandbox/ @databricks/eng-deco-cli @databricks/eng-sandbox +/acceptance/cmd/sandbox/ @databricks/eng-deco-cli @databricks/eng-sandbox + +# Auth +/cmd/auth/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/auth/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/acceptance/auth/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +# Filesystem & sync +/cmd/fs/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/sync/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/filer/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/sync/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +# Core CLI infrastructure +/cmd/root/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/version/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/completion/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/configure/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/cache/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/api/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/selftest/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/cmd/psql/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/psql/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +# Libs (general) +/libs/databrickscfg/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/env/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/flags/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/cmdio/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/log/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/telemetry/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/process/ @databricks/eng-deco-cli @databricks/eng-deco-platform +/libs/git/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +# Integration tests +/integration/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +/integration/cmd/environments/ @databricks/eng-deco-cli @databricks/eng-deco-ax + +# Internal +/internal/ @databricks/eng-deco-cli @databricks/eng-deco-platform + +# AI tools +/cmd/aitools/ @databricks/eng-deco-cli @databricks/eng-apps-devex @databricks/eng-deco-ax @lennartkats-db +/libs/aitools/ @databricks/eng-deco-cli @databricks/eng-apps-devex @databricks/eng-deco-ax @lennartkats-db + +# CLI compatibility manifest +/internal/build/cli-compat.json @databricks/eng-deco-cli @databricks/eng-apps-devex @databricks/eng-deco-platform +/libs/clicompat/ @databricks/eng-deco-cli @databricks/eng-apps-devex @databricks/eng-deco-platform + +# Experimental +/experimental/air/ @databricks/eng-deco-cli @databricks/eng-ai-custom-training +/acceptance/experimental/air/ @databricks/eng-deco-cli @databricks/eng-ai-custom-training +/experimental/aitools/ @databricks/eng-deco-cli @databricks/eng-apps-devex @lennartkats-db diff --git a/.github/OWNERS b/.github/OWNERS deleted file mode 100644 index a0b9aa88c8a..00000000000 --- a/.github/OWNERS +++ /dev/null @@ -1,83 +0,0 @@ -# Maintainers (can approve any PR) -* @andrewnester @anton-107 @denik @pietern @shreyas-goenka @simonfaltum @renaudhartert-db @janniklasrose @lennartkats-db @rugpanov @rclarey - -# Bundles -/bundle/ team:bundle -/cmd/bundle/ team:bundle -/acceptance/bundle/ team:bundle -/libs/template/ team:bundle - -# Pipelines -/cmd/pipelines/ @jefferycheng1 @kanterov @lennartkats-db -/acceptance/pipelines/ @jefferycheng1 @kanterov @lennartkats-db - -# Labs -/cmd/labs/ @alexott @asnare - -# Local environments / DB Connect -/libs/localenv/ team:ide -/cmd/environments/ team:ide -/acceptance/localenv/ team:ide - -# Apps -/cmd/apps/ team:eng-apps-devex -/cmd/workspace/apps/ team:eng-apps-devex -/libs/apps/ team:eng-apps-devex -/acceptance/apps/ team:eng-apps-devex - -# Sandbox -/cmd/sandbox/ team:eng-sandbox -/acceptance/cmd/sandbox/ team:eng-sandbox - -# Auth -/cmd/auth/ team:platform -/libs/auth/ team:platform -/acceptance/auth/ team:platform - -# Filesystem & sync -/cmd/fs/ team:platform -/cmd/sync/ team:platform -/libs/filer/ team:platform -/libs/sync/ team:platform - -# Core CLI infrastructure -/cmd/root/ team:platform -/cmd/version/ team:platform -/cmd/completion/ team:platform -/cmd/configure/ team:platform -/cmd/cache/ team:platform -/cmd/api/ team:platform -/cmd/selftest/ team:platform -/cmd/psql/ team:platform -/libs/psql/ team:platform - -# Libs (general) -/libs/databrickscfg/ team:platform -/libs/env/ team:platform -/libs/flags/ team:platform -/libs/cmdio/ team:platform -/libs/log/ team:platform -/libs/telemetry/ team:platform -/libs/process/ team:platform -/libs/git/ team:platform - -# Integration tests -/integration/ team:platform - -/integration/cmd/environments/ team:ide - -# Internal -/internal/ team:platform - -# AI tools -/cmd/aitools/ team:eng-apps-devex team:ide @lennartkats-db -/libs/aitools/ team:eng-apps-devex team:ide @lennartkats-db - -# CLI compatibility manifest -/internal/build/cli-compat.json team:eng-apps-devex team:platform -/libs/clicompat/ team:eng-apps-devex team:platform - -# Experimental -/experimental/air/ team:ai-training -/acceptance/experimental/air/ team:ai-training -/experimental/aitools/ team:eng-apps-devex @lennartkats-db diff --git a/.github/OWNERTEAMS b/.github/OWNERTEAMS deleted file mode 100644 index f7bd2a101ab..00000000000 --- a/.github/OWNERTEAMS +++ /dev/null @@ -1,20 +0,0 @@ -# Team aliases for OWNERS file. -# Use "team:" in OWNERS to reference a team defined here. -# Format: team: @member1 @member2 ... -# -# Keep these in sync with actual GitHub team rosters. GITHUB_TOKEN can't -# resolve org team membership via the API, so this file is the source of -# truth for the maintainer-approval workflow. -# -# GitHub team pages: -# bundle: https://github.com/orgs/databricks/teams/cli-maintainers -# platform: https://github.com/orgs/databricks/teams/cli-platform -# eng-apps-devex: https://github.com/orgs/databricks/teams/eng-apps-devex -# eng-sandbox: https://github.com/orgs/databricks/teams/eng-sandbox - -team:bundle @andrewnester @anton-107 @denik @janniklasrose @lennartkats-db @pietern @shreyas-goenka -team:platform @simonfaltum @renaudhartert-db @hectorcast-db @parthban-db @tanmay-db @Divyansh-db @tejaskochar-db @mihaimitrea-db @chrisst @rauchy -team:eng-apps-devex @fjakobs @Shridhad @atilafassina @keugenek @igrekun @pkosiec @MarioCadenas @pffigueiredo @ditadi @calvarjorge -team:eng-sandbox @pietern @shuochen0311 @akshaysingla-db @anwell-db @samhuan-db -team:ide @rugpanov @rclarey @anton-107 @misha-db @parthban-db -team:ai-training @apeforest @bfontain @panchalhp-db @vinchenzo-db @maggiewang-db @ben-hansen-db @pardis-beikzadeh-db @caroline-db diff --git a/.github/scripts/owners.js b/.github/scripts/owners.js deleted file mode 100644 index 158b872ef2b..00000000000 --- a/.github/scripts/owners.js +++ /dev/null @@ -1,273 +0,0 @@ -const fs = require("fs"); -const path = require("path"); - -/** - * Read a file and return non-empty, non-comment lines split by whitespace. - * Returns [] if the file does not exist. - * - * @param {string} filePath - * @returns {string[][]} array of whitespace-split tokens per line - */ -function readDataLines(filePath) { - let content; - try { - content = fs.readFileSync(filePath, "utf-8"); - } catch (e) { - if (e.code === "ENOENT") return []; - throw e; - } - const result = []; - for (const raw of content.split("\n")) { - const line = raw.trim(); - if (!line || line.startsWith("#")) continue; - const parts = line.split(/\s+/); - if (parts.length >= 2) result.push(parts); - } - return result; -} - -/** - * Parse an OWNERTEAMS file into a map of team aliases. - * Format: "team: @member1 @member2 ..." - * Returns Map where key is "team:" and value is member logins. - * - * @param {string} filePath - absolute path to the OWNERTEAMS file - * @returns {Map} - */ -function parseOwnerTeams(filePath) { - const teams = new Map(); - for (const parts of readDataLines(filePath)) { - if (!parts[0].startsWith("team:")) continue; - const members = parts.slice(1).filter((p) => p.startsWith("@")).map((p) => p.slice(1)); - teams.set(parts[0], members); - } - return teams; -} - -/** - * Parse an OWNERS file (same format as CODEOWNERS). - * Returns array of { pattern, owners } rules. - * - * If an OWNERTEAMS file exists alongside the OWNERS file, "team:" - * tokens are expanded to their member lists. - * - * By default, team refs (org/team) are filtered out and @ is stripped. - * Pass { includeTeams: true } to keep team refs (with @ stripped). - * - * @param {string} filePath - absolute path to the OWNERS file - * @param {{ includeTeams?: boolean }} [opts] - * @returns {Array<{pattern: string, owners: string[]}>} - */ -function parseOwnersFile(filePath, opts) { - const includeTeams = opts && opts.includeTeams; - const teamsPath = path.join(path.dirname(filePath), "OWNERTEAMS"); - const teams = parseOwnerTeams(teamsPath); - const rules = []; - for (const parts of readDataLines(filePath)) { - const pattern = parts[0]; - const owners = []; - for (const p of parts.slice(1)) { - if (p.startsWith("team:") && teams.has(p)) { - owners.push(...teams.get(p)); - } else if (p.startsWith("@") && (includeTeams || !p.includes("/"))) { - owners.push(p.slice(1)); - } - } - rules.push({ pattern, owners }); - } - return rules; -} - -/** - * Match a filepath against an OWNERS pattern. - * Supports: "*" (catch-all), "/dir/" (prefix), "/path/file" (exact). - */ -function ownersMatch(pattern, filepath) { - if (pattern === "*") return true; - let p = pattern; - if (p.startsWith("/")) p = p.slice(1); - if (p.endsWith("/")) return filepath.startsWith(p); - return filepath === p; -} - -/** - * Find owners for a file. Last match wins, like CODEOWNERS. - * @returns {string[]} owner logins - */ -function findOwners(filepath, rules) { - let matched = []; - for (const rule of rules) { - if (ownersMatch(rule.pattern, filepath)) { - matched = rule.owners; - } - } - return matched; -} - -/** - * Get maintainers from the * catch-all rule. - * @returns {string[]} logins - */ -function getMaintainers(rules) { - const catchAll = rules.find((r) => r.pattern === "*"); - return catchAll ? catchAll.owners : []; -} - -/** - * 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; -} - -/** - * Parse OWNERS into raw rules WITHOUT expanding team aliases. - * Unlike parseOwnersFile, this keeps the original tokens ("team:x", "@user") - * so the validator can tell defined from undefined teams and count owners. - * - * @param {string} filePath - absolute path to the OWNERS file - * @returns {Array<{pattern: string, tokens: string[]}>} - */ -function parseOwnersRules(filePath) { - return readDataLines(filePath).map((parts) => ({ - pattern: parts[0], - tokens: parts.slice(1), - })); -} - -/** - * Parse the GitHub team-page URLs declared in the OWNERTEAMS header comment. - * Format: "# : https://github.com/orgs/databricks/teams/" - * Returns the set of "team:" aliases that have a documented page. - * - * @param {string} filePath - absolute path to the OWNERTEAMS file - * @returns {Set} - */ -function parseTeamPageUrls(filePath) { - let content; - try { - content = fs.readFileSync(filePath, "utf-8"); - } catch (e) { - if (e.code === "ENOENT") return new Set(); - throw e; - } - const pages = new Set(); - for (const raw of content.split("\n")) { - const m = raw.match(/^#\s*([a-z0-9-]+):\s*https?:\/\//); - if (m) pages.add("team:" + m[1]); - } - return pages; -} - -/** - * Validate OWNERS and OWNERTEAMS for internal consistency. - * - * Errors (block CI): - * - a rule references a "team:" alias not defined in OWNERTEAMS - * - a rule resolves to zero owners (only a maintainer could ever approve it) - * - a rule maps a path that does not exist in the repository - * - * Warnings (reported, non-blocking): a defined team has no team-page URL in the - * OWNERTEAMS header. A team may legitimately predate its GitHub team page, so - * this never blocks a merge. - * - * fileExists is injectable so tests can validate synthetic rules without a tree. - * - * @param {string} ownersPath - * @param {string} teamsPath - * @param {{ repoRoot?: string, fileExists?: (p: string) => boolean }} [opts] - * @returns {{ errors: string[], warnings: string[] }} - */ -function validateOwners(ownersPath, teamsPath, opts) { - const repoRoot = (opts && opts.repoRoot) || process.cwd(); - const fileExists = (opts && opts.fileExists) || ((p) => fs.existsSync(p)); - const teams = parseOwnerTeams(teamsPath); - const pageUrls = parseTeamPageUrls(teamsPath); - const errors = []; - const warnings = []; - - for (const { pattern, tokens } of parseOwnersRules(ownersPath)) { - let ownerCount = 0; - let undefinedTeam = false; - for (const token of tokens) { - if (token.startsWith("team:")) { - if (teams.has(token)) { - ownerCount += teams.get(token).length; - } else { - errors.push(`rule "${pattern}" references undefined team "${token}"; define it in .github/OWNERTEAMS`); - undefinedTeam = true; - } - } else if (token.startsWith("@")) { - ownerCount += 1; - } - } - // Skip the zero-owner error when an undefined team already explains it. - if (ownerCount === 0 && !undefinedTeam) { - errors.push(`rule "${pattern}" resolves to zero owners`); - } - if (pattern !== "*") { - const rel = pattern.replace(/^\//, "").replace(/\/$/, ""); - if (rel && !fileExists(path.join(repoRoot, rel))) { - errors.push(`rule "${pattern}" maps a path that does not exist in the repo`); - } - } - } - - for (const team of teams.keys()) { - if (!pageUrls.has(team)) { - warnings.push(`team "${team}" has no GitHub team-page URL in the .github/OWNERTEAMS header comment`); - } - } - - return { errors, warnings }; -} - -module.exports = { - parseOwnerTeams, - parseOwnersFile, - parseOwnersRules, - parseTeamPageUrls, - ownersMatch, - findOwners, - getMaintainers, - getOwnershipGroups, - validateOwners, -}; - -// CLI entrypoint: `node .github/scripts/owners.js validate` -if (require.main === module) { - if (process.argv[2] !== "validate") { - console.error("usage: node .github/scripts/owners.js validate"); - process.exit(2); - } - const root = process.cwd(); - const { errors, warnings } = validateOwners( - path.join(root, ".github", "OWNERS"), - path.join(root, ".github", "OWNERTEAMS"), - { repoRoot: root }, - ); - for (const w of warnings) console.warn(`warning: ${w}`); - for (const e of errors) console.error(`error: ${e}`); - if (errors.length > 0) { - console.error(`OWNERS validation failed: ${errors.length} error(s), ${warnings.length} warning(s)`); - process.exit(1); - } - console.log(`OWNERS validation passed: ${warnings.length} warning(s)`); -} diff --git a/.github/scripts/owners.test.js b/.github/scripts/owners.test.js deleted file mode 100644 index ccfc942cf57..00000000000 --- a/.github/scripts/owners.test.js +++ /dev/null @@ -1,517 +0,0 @@ -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 { - parseOwnerTeams, - ownersMatch, - parseOwnersFile, - parseOwnersRules, - parseTeamPageUrls, - findOwners, - getMaintainers, - getOwnershipGroups, - validateOwners, -} = 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, "*"); - }); -}); - -// --- parseOwnerTeams --- - -describe("parseOwnerTeams", () => { - let tmpDir; - - before(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "ownerteams-test-")); - }); - - after(() => { - fs.rmSync(tmpDir, { recursive: true }); - }); - - it("parses team definitions", () => { - const teamsPath = path.join(tmpDir, "OWNERTEAMS"); - fs.writeFileSync(teamsPath, "team:platform @alice @bob @carol\n"); - const teams = parseOwnerTeams(teamsPath); - assert.equal(teams.size, 1); - assert.deepEqual(teams.get("team:platform"), ["alice", "bob", "carol"]); - }); - - it("parses multiple teams", () => { - const teamsPath = path.join(tmpDir, "OWNERTEAMS"); - fs.writeFileSync(teamsPath, "team:platform @alice @bob\nteam:bundle @carol @dave\n"); - const teams = parseOwnerTeams(teamsPath); - assert.equal(teams.size, 2); - assert.deepEqual(teams.get("team:platform"), ["alice", "bob"]); - assert.deepEqual(teams.get("team:bundle"), ["carol", "dave"]); - }); - - it("skips comments and blank lines", () => { - const teamsPath = path.join(tmpDir, "OWNERTEAMS"); - fs.writeFileSync(teamsPath, "# comment\n\nteam:platform @alice\n"); - const teams = parseOwnerTeams(teamsPath); - assert.equal(teams.size, 1); - }); - - it("returns empty map if file does not exist", () => { - const teams = parseOwnerTeams(path.join(tmpDir, "NONEXISTENT")); - assert.equal(teams.size, 0); - }); -}); - -// --- parseOwnersFile with team aliases --- - -describe("parseOwnersFile with OWNERTEAMS", () => { - let tmpDir; - let ownersPath; - let teamsPath; - - before(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-teams-test-")); - ownersPath = path.join(tmpDir, "OWNERS"); - teamsPath = path.join(tmpDir, "OWNERTEAMS"); - }); - - after(() => { - fs.rmSync(tmpDir, { recursive: true }); - }); - - it("expands team aliases to members", () => { - fs.writeFileSync(teamsPath, "team:platform @alice @bob\n"); - fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform\n"); - const rules = parseOwnersFile(ownersPath); - assert.equal(rules.length, 1); - assert.deepEqual(rules[0].owners, ["alice", "bob"]); - }); - - it("mixes team aliases with individual owners", () => { - fs.writeFileSync(teamsPath, "team:platform @alice @bob\n"); - fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform @carol\n"); - const rules = parseOwnersFile(ownersPath); - assert.equal(rules.length, 1); - assert.deepEqual(rules[0].owners, ["alice", "bob", "carol"]); - }); - - it("unknown team alias is ignored", () => { - fs.writeFileSync(teamsPath, "team:platform @alice\n"); - fs.writeFileSync(ownersPath, "/cmd/auth/ team:unknown @bob\n"); - const rules = parseOwnersFile(ownersPath); - assert.deepEqual(rules[0].owners, ["bob"]); - }); - - it("works without OWNERTEAMS file", () => { - const tmpDir2 = fs.mkdtempSync(path.join(os.tmpdir(), "owners-noteams-")); - const ownersPath2 = path.join(tmpDir2, "OWNERS"); - fs.writeFileSync(ownersPath2, "* @alice\n"); - const rules = parseOwnersFile(ownersPath2); - assert.deepEqual(rules[0].owners, ["alice"]); - fs.rmSync(tmpDir2, { recursive: true }); - }); -}); - -// --- 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("*")); - }); -}); - -// --- parseOwnersRules --- - -describe("parseOwnersRules", () => { - let tmpDir; - let ownersPath; - - before(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-raw-")); - ownersPath = path.join(tmpDir, "OWNERS"); - }); - - after(() => { - fs.rmSync(tmpDir, { recursive: true }); - }); - - it("keeps team aliases and @users un-expanded", () => { - fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform @carol\n* @alice\n"); - const rules = parseOwnersRules(ownersPath); - assert.equal(rules.length, 2); - assert.deepEqual(rules[0], { - pattern: "/cmd/auth/", - tokens: ["team:platform", "@carol"], - }); - assert.deepEqual(rules[1], { pattern: "*", tokens: ["@alice"] }); - }); -}); - -// --- parseTeamPageUrls --- - -describe("parseTeamPageUrls", () => { - let tmpDir; - let teamsPath; - - before(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "team-urls-")); - teamsPath = path.join(tmpDir, "OWNERTEAMS"); - }); - - after(() => { - fs.rmSync(tmpDir, { recursive: true }); - }); - - it("collects team-page URLs from header comments", () => { - fs.writeFileSync( - teamsPath, - [ - "# GitHub team pages:", - "# platform: https://github.com/orgs/databricks/teams/cli-platform", - "# bundle: https://github.com/orgs/databricks/teams/cli-maintainers", - "team:platform @alice", - ].join("\n") - ); - const pages = parseTeamPageUrls(teamsPath); - assert.equal(pages.size, 2); - assert.ok(pages.has("team:platform")); - assert.ok(pages.has("team:bundle")); - }); - - it("ignores header lines that are not team-page URLs", () => { - fs.writeFileSync( - teamsPath, - [ - '# Use "team:" in OWNERS to reference a team.', - "# Format: team: @member1", - "team:platform @alice", - ].join("\n") - ); - assert.equal(parseTeamPageUrls(teamsPath).size, 0); - }); -}); - -// --- validateOwners --- - -describe("validateOwners", () => { - let tmpDir; - let ownersPath; - let teamsPath; - const allExist = () => true; - - before(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-validate-")); - ownersPath = path.join(tmpDir, "OWNERS"); - teamsPath = path.join(tmpDir, "OWNERTEAMS"); - }); - - after(() => { - fs.rmSync(tmpDir, { recursive: true }); - }); - - function write(owners, teams) { - fs.writeFileSync(ownersPath, owners); - fs.writeFileSync(teamsPath, teams); - } - - it("passes a consistent OWNERS/OWNERTEAMS pair", () => { - write( - "* @alice\n/cmd/auth/ team:platform\n", - "# platform: https://github.com/orgs/databricks/teams/cli-platform\nteam:platform @bob @carol\n" - ); - const { errors, warnings } = validateOwners(ownersPath, teamsPath, { - fileExists: allExist, - }); - assert.deepEqual(errors, []); - assert.deepEqual(warnings, []); - }); - - it("errors on a team alias not defined in OWNERTEAMS", () => { - write("* @alice\n/cmd/auth/ team:platfrom\n", "team:platform @bob\n"); - const { errors } = validateOwners(ownersPath, teamsPath, { - fileExists: allExist, - }); - assert.equal(errors.length, 1); - assert.match(errors[0], /undefined team "team:platfrom"/); - }); - - it("does not also report zero owners when a team is undefined", () => { - write("/cmd/auth/ team:nope\n", "team:platform @bob\n"); - const { errors } = validateOwners(ownersPath, teamsPath, { - fileExists: allExist, - }); - assert.equal(errors.length, 1); - assert.match(errors[0], /undefined team/); - }); - - it("errors when a rule resolves to zero owners (org team missing @)", () => { - write("/cmd/auth/ databricks/eng-apps-devex\n", "team:platform @bob\n"); - const { errors } = validateOwners(ownersPath, teamsPath, { - fileExists: allExist, - }); - assert.equal(errors.length, 1); - assert.match(errors[0], /zero owners/); - }); - - it("errors when a rule maps a path that does not exist", () => { - write( - "* @alice\n/acceptance/ghost/ team:platform\n", - "# platform: https://github.com/orgs/databricks/teams/cli-platform\nteam:platform @bob\n" - ); - const { errors } = validateOwners(ownersPath, teamsPath, { - fileExists: (p) => !p.includes("ghost"), - }); - assert.equal(errors.length, 1); - assert.match(errors[0], /does not exist/); - }); - - it("does not path-check the * catch-all rule", () => { - write("* @alice\n", "team:platform @bob\n"); - const { errors } = validateOwners(ownersPath, teamsPath, { - fileExists: () => false, - }); - assert.deepEqual(errors, []); - }); - - it("warns (does not error) when a defined team has no team-page URL", () => { - write("* @alice\n/cmd/auth/ team:newteam\n", "team:newteam @bob\n"); - const { errors, warnings } = validateOwners(ownersPath, teamsPath, { - fileExists: allExist, - }); - assert.deepEqual(errors, []); - assert.equal(warnings.length, 1); - assert.match(warnings[0], /team-page URL/); - }); -}); diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js deleted file mode 100644 index 93ce4de78c7..00000000000 --- a/.github/workflows/maintainer-approval.js +++ /dev/null @@ -1,598 +0,0 @@ -const path = require("path"); -const { execFileSync } = require("child_process"); -const { - parseOwnersFile, - findOwners, - getMaintainers, - getOwnershipGroups, -} = require("../scripts/owners"); - -// --- Approval helpers --- - -/** - * 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) { - return false; - } - 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; - } -} - -/** - * Find which approver (if any) satisfies a group's ownership requirement. - * Returns the login of the first matching approver, or null. - */ -async function findGroupApprover(owners, approverLogins, github, org, core) { - const approverSet = new Set(approverLogins.map(l => l.toLowerCase())); - for (const owner of owners) { - if (owner.includes("/")) { - const teamSlug = owner.split("/")[1]; - for (const approver of approverLogins) { - if (await isTeamMember(github, org, teamSlug, approver, core)) { - return approver; - } - } - } else if (approverSet.has(owner.toLowerCase())) { - return owner; - } - } - return null; -} - -/** - * Per-path approval check. Each ownership group needs at least one - * approval from its owners. Files matching only "*" require a maintainer. - * Returns groups, approvedBy map, and coverage info. - */ -async function checkPerPathApproval(files, rulesWithTeams, approverLogins, github, org, core) { - const groups = getOwnershipGroups(files.map(f => f.filename), rulesWithTeams); - const approvedBy = new Map(); - - if (groups.has("*")) { - // Still check non-wildcard groups for comment building - for (const [pattern, { owners }] of groups) { - if (pattern === "*") continue; - const approver = await findGroupApprover(owners, approverLogins, github, org, core); - if (approver) approvedBy.set(pattern, approver); - } - return { - allCovered: false, - hasWildcardFiles: true, - wildcardFiles: groups.get("*").files, - groups, - approvedBy, - }; - } - - const uncovered = []; - for (const [pattern, { owners }] of groups) { - const approver = await findGroupApprover(owners, approverLogins, github, org, core); - if (approver) { - approvedBy.set(pattern, approver); - } else { - uncovered.push({ pattern, owners }); - } - } - return { allCovered: uncovered.length === 0, uncovered, groups, approvedBy }; -} - -// --- Git history & scoring helpers --- - -const MENTION_REVIEWERS = false; -const OWNERS_LINK = "[OWNERS](/databricks/cli/blob/main/.github/OWNERS)"; -const MARKER = ""; -const STATUS_CONTEXT = "maintainer-approval"; - -const loginCache = {}; - -function classifyFile(filepath, totalFiles) { - const base = path.basename(filepath); - if (base.startsWith("out.") || base === "output.txt") { - return 0.01 / Math.max(totalFiles, 1); - } - if (filepath.startsWith("acceptance/") || filepath.startsWith("integration/")) { - return 0.2; - } - if (filepath.endsWith("_test.go")) return 0.3; - return filepath.endsWith(".go") ? 1.0 : 0.5; -} - -function gitLog(filepath) { - try { - const out = execFileSync( - "git", - ["log", "-50", "--no-merges", "--since=12 months ago", "--format=%H|%an|%aI", "--", filepath], - { encoding: "utf-8" } - ); - const entries = []; - for (const line of out.split("\n")) { - const trimmed = line.trim(); - if (!trimmed) continue; - const parts = trimmed.split("|", 3); - if (parts.length !== 3) continue; - const date = new Date(parts[2]); - if (isNaN(date.getTime())) continue; - entries.push({ sha: parts[0], name: parts[1], date }); - } - return entries; - } catch { - return []; - } -} - -async function resolveLogin(github, owner, repo, sha, authorName) { - if (authorName in loginCache) return loginCache[authorName]; - try { - const { data } = await github.rest.repos.getCommit({ owner, repo, ref: sha }); - const login = data.author?.login || null; - loginCache[authorName] = login; - return login; - } catch { - loginCache[authorName] = null; - return null; - } -} - -function parseOwnersForFiles(changedFiles, ownersPath) { - const rules = parseOwnersFile(ownersPath, { includeTeams: true }); - const allOwners = new Set(); - for (const filepath of changedFiles) { - for (const o of findOwners(filepath, rules)) allOwners.add(o); - } - return Array.from(allOwners).sort(); -} - -async function scoreContributors(files, prAuthor, now, github, owner, repo) { - const scores = {}; - const dirScores = {}; - let scoredCount = 0; - const authorLogin = (prAuthor || "").toLowerCase(); - const totalFiles = files.length; - - for (const filepath of files) { - const weight = classifyFile(filepath, totalFiles); - let history = gitLog(filepath); - if (history.length === 0) { - const parent = path.dirname(filepath); - if (parent && parent !== ".") { - history = gitLog(parent); - } - } - if (history.length === 0) continue; - - const topDir = path.dirname(filepath) || "."; - let fileContributed = false; - for (const { sha, name, date } of history) { - if (name.endsWith("[bot]")) continue; - const login = await resolveLogin(github, owner, repo, sha, name); - if (!login || login.toLowerCase() === authorLogin) continue; - const daysAgo = Math.max(0, (now - date) / 86400000); - const s = weight * Math.pow(0.5, daysAgo / 150); - scores[login] = (scores[login] || 0) + s; - if (!dirScores[login]) dirScores[login] = {}; - dirScores[login][topDir] = (dirScores[login][topDir] || 0) + s; - fileContributed = true; - } - if (fileContributed) scoredCount++; - } - return { scores, dirScores, scoredCount }; -} - -function topDirs(ds, n = 3) { - return Object.entries(ds || {}) - .sort((a, b) => b[1] - a[1]) - .slice(0, n) - .map(([d]) => d); -} - -function fmtReviewer(login, dirs) { - const dirList = dirs.map((d) => `\`${d}/\``).join(", "); - return `- ${fmtLogin(login)} -- recent work in ${dirList}`; -} - -function selectReviewers(ss) { - if (ss.length === 0) return []; - const out = [ss[0]]; - if (ss.length >= 2 && ss[0][1] < 1.5 * ss[1][1]) { - out.push(ss[1]); - if (ss.length >= 3 && ss[1][1] < 1.5 * ss[2][1]) { - out.push(ss[2]); - } - } - return out; -} - -function fmtEligible(owners) { - return owners.map((o) => fmtLogin(o)).join(", "); -} - -function fmtLogin(login) { - if (MENTION_REVIEWERS) return `@${login}`; - return `\`@${login}\``; -} - -async function countRecentReviews(github, owner, repo, logins, days = 30) { - const since = new Date(Date.now() - days * 86400000) - .toISOString() - .slice(0, 10); - const counts = {}; - for (const login of logins) { - try { - const { data } = await github.rest.search.issuesAndPullRequests({ - q: `repo:${owner}/${repo} reviewed-by:${login} is:pr created:>${since}`, - }); - counts[login] = data.total_count; - } catch { - // skip on error - } - } - return counts; -} - -async function selectRoundRobin(github, owner, repo, eligibleOwners, prAuthor) { - const candidates = eligibleOwners - .filter((o) => !o.includes("/") && o.toLowerCase() !== (prAuthor || "").toLowerCase()); - if (candidates.length === 0) return null; - const counts = await countRecentReviews(github, owner, repo, candidates); - if (Object.keys(counts).length === 0) { - return candidates[Math.floor(Math.random() * candidates.length)]; - } - return candidates.reduce((best, c) => - (counts[c] || 0) < (counts[best] || 0) ? c : best - ); -} - -// --- Comment builders --- - -function fmtFileList(files) { - if (files.length < 4) { - return `Files: ${files.map(f => `\`${f}\``).join(", ")}`; - } - return `${files.length} files changed`; -} - -function buildPendingPerGroupComment(groups, scores, dirScores, approvedBy, maintainers, prAuthor) { - const authorLower = (prAuthor || "").toLowerCase(); - const lines = [MARKER, "## Approval status: pending", ""]; - - for (const [pattern, { owners, files }] of groups) { - if (pattern === "*") continue; - - const approver = approvedBy.get(pattern); - if (approver) { - lines.push(`### \`${pattern}\` - approved by ${fmtLogin(approver)}`); - } else { - lines.push(`### \`${pattern}\` - needs approval`); - } - lines.push(fmtFileList(files)); - - 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 => fmtLogin(t)).join(", ")}`); - } - - if (!approver && individuals.length > 0) { - const scored = individuals.map(o => [o, scores[o] || 0]).sort((a, b) => b[1] - a[1]); - if (scored[0][1] > 0) { - lines.push(`Suggested: ${fmtLogin(scored[0][0])}`); - 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(fmtFileList(starGroup.files)); - - 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 => fmtLogin(m)) - .join(", "); - - lines.push( - `Any maintainer (${maintainerList}) can approve all areas.`, - `See ${OWNERS_LINK} for ownership rules.` - ); - - return lines.join("\n") + "\n"; -} - -function buildSingleDomainPendingComment(sortedScores, dirScores, scoredCount, eligibleOwners, prAuthor, roundRobinReviewer) { - const reviewers = selectReviewers(sortedScores); - const suggestedLogins = new Set(reviewers.map(([login]) => login.toLowerCase())); - const eligible = eligibleOwners.filter( - o => o.toLowerCase() !== (prAuthor || "").toLowerCase() && !suggestedLogins.has(o.toLowerCase()) - ); - - const lines = [MARKER, "## Waiting for approval", ""]; - - if (reviewers.length > 0) { - lines.push("Based on git history, these people are best suited to review:", ""); - for (const [login] of reviewers) { - lines.push(fmtReviewer(login, topDirs(dirScores[login]))); - } - lines.push(""); - } else if (roundRobinReviewer) { - lines.push( - "Could not determine reviewers from git history.", - `Round-robin suggestion: ${fmtLogin(roundRobinReviewer)}`, - "" - ); - } - - if (eligible.length > 0) { - lines.push(`Eligible reviewers: ${fmtEligible(eligible)}`, ""); - } - - lines.push(`Suggestions based on git history. See ${OWNERS_LINK} for ownership rules.`); - return lines.join("\n") + "\n"; -} - -// --- Comment management --- - -const LEGACY_MARKER = ""; - -/** - * Delete all marker and legacy marker comments from the PR. - * Used on success paths to clean up stale pending comments. - */ -async function deleteMarkerComments(github, owner, repo, prNumber) { - const comments = await github.paginate(github.rest.issues.listComments, { - owner, repo, issue_number: prNumber, - }); - for (const c of comments) { - if (c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER))) { - await github.rest.issues.deleteComment({ - owner, repo, comment_id: c.id, - }); - } - } -} - -/** - * Create or edit the marker comment. Skips the edit if the body is unchanged. - * Cleans up duplicate or legacy marker comments, keeping only the first one. - */ -async function upsertComment(github, owner, repo, prNumber, newBody) { - const comments = await github.paginate(github.rest.issues.listComments, { - owner, repo, issue_number: prNumber, - }); - const markerComments = comments.filter(c => - c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER)) - ); - - if (markerComments.length > 0) { - const existing = markerComments[0]; - - // Clean up duplicates (legacy or accidental), keep the first. - for (const c of markerComments.slice(1)) { - await github.rest.issues.deleteComment({ - owner, repo, comment_id: c.id, - }); - } - - // Skip if body is unchanged. - if (existing.body === newBody) return; - - await github.rest.issues.updateComment({ - owner, repo, comment_id: existing.id, body: newBody, - }); - return; - } - - await github.rest.issues.createComment({ - owner, repo, issue_number: prNumber, body: newBody, - }); -} - -// --- Main --- - -module.exports = async ({ github, context, core }) => { - const ownersPath = path.join( - process.env.GITHUB_WORKSPACE, - ".github", - "OWNERS" - ); - const rulesWithTeams = parseOwnersFile(ownersPath, { includeTeams: true }); - const maintainers = getMaintainers(rulesWithTeams); - - if (maintainers.length === 0) { - core.setFailed( - "Could not determine maintainers from .github/OWNERS (no * rule found)." - ); - return; - } - - const { pull_request: pr } = context.payload; - const owner = context.repo.owner; - const repo = context.repo.repo; - const prNumber = context.issue.number; - const authorLogin = pr?.user?.login; - const sha = pr.head.sha; - const checkParams = { - owner: context.repo.owner, - repo: context.repo.repo, - head_sha: sha, - name: STATUS_CONTEXT, - }; - - const reviews = await github.paginate(github.rest.pulls.listReviews, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.issue.number, - }); - - // Maintainer approval -> success with simple comment - const maintainerApproval = reviews.find( - ({ state, user }) => - state === "APPROVED" && user && maintainers.includes(user.login) - ); - if (maintainerApproval) { - const approver = maintainerApproval.user.login; - core.info(`Maintainer approval from @${approver}`); - await github.rest.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: `Approved by @${approver}` }, - }); - await deleteMarkerComments(github, owner, repo, prNumber); - return; - } - - // Maintainer-authored PR with any approval -> success - 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.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: "Approved (maintainer-authored PR)" }, - }); - await deleteMarkerComments(github, owner, repo, prNumber); - return; - } - } - - // Gather approved logins (excluding the PR author). - 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, - repo: context.repo.repo, - pull_number: context.issue.number, - }); - - const result = await checkPerPathApproval( - files, - rulesWithTeams, - approverLogins, - github, - context.repo.owner, - core - ); - - // Approved PRs get a success check run and return early. - // Pending PRs intentionally create NO check run or status. The required - // status check "maintainer-approval" stays as "Expected" (yellow dot) in - // the GitHub UI, which blocks the merge until approval is granted. - if (result.allCovered && approverLogins.length > 0) { - core.info("All ownership groups have per-path approval."); - await github.rest.checks.create({ - ...checkParams, - status: "completed", - conclusion: "success", - output: { title: STATUS_CONTEXT, summary: "All ownership groups approved" }, - }); - await deleteMarkerComments(github, owner, repo, prNumber); - return; - } - - if (result.hasWildcardFiles) { - const fileList = result.wildcardFiles.join(", "); - core.info( - `Files need maintainer review: ${fileList}. ` + - `Maintainers: ${maintainers.join(", ")}` - ); - } else if (result.uncovered && result.uncovered.length > 0) { - const groupList = result.uncovered - .map(({ pattern, owners }) => `${pattern} (needs: ${owners.join(", ")})`) - .join("; "); - core.info( - `Needs approval: ${groupList}. ` + - `Alternatively, any maintainer can approve: ${maintainers.join(", ")}.` - ); - } else { - core.info(`Waiting for maintainer approval: ${maintainers.join(", ")}`); - } - - // Score contributors via git history - const fileNames = files.map(f => f.filename); - const now = new Date(); - const { scores, dirScores, scoredCount } = await scoreContributors( - fileNames, - authorLogin, - now, - github, - owner, - repo - ); - const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); - - // Build pending comment with reviewer suggestions. - let comment; - const groups = result.groups; - - if (groups.size >= 2) { - comment = buildPendingPerGroupComment( - groups, scores, dirScores, result.approvedBy, maintainers, authorLogin - ); - } else { - const eligible = parseOwnersForFiles(fileNames, ownersPath); - let roundRobin = null; - if (selectReviewers(sortedScores).length === 0 && eligible.length > 0) { - roundRobin = await selectRoundRobin(github, owner, repo, eligible, authorLogin); - } - comment = buildSingleDomainPendingComment( - sortedScores, dirScores, scoredCount, eligible, authorLogin, roundRobin - ); - } - - core.info(comment); - await upsertComment(github, owner, repo, prNumber, comment); -}; diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js deleted file mode 100644 index 2866dc9d3d7..00000000000 --- a/.github/workflows/maintainer-approval.test.js +++ /dev/null @@ -1,561 +0,0 @@ -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 runModule = require("./maintainer-approval"); - -// --- Test helpers --- - -function makeTmpOwners(content, ownerTeamsContent) { - 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); - if (ownerTeamsContent) { - fs.writeFileSync(path.join(ghDir, "OWNERTEAMS"), ownerTeamsContent); - } - return tmpDir; -} - -const OWNERTEAMS_CONTENT = "team:eng-apps-devex @teamdev1 @teamdev2\n"; - -const OWNERS_CONTENT = [ - "* @maintainer1 @maintainer2", - "/cmd/pipelines/ @jefferycheng1 @kanterov", - "/cmd/apps/ team: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: { - number: prNumber, - 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] } - * @param {Array} opts.existingComments - Existing PR comments to return - */ -function makeGithub({ reviews = [], files = [], teamMembers = {}, existingComments = [] } = {}) { - const listReviews = Symbol("listReviews"); - const listFiles = Symbol("listFiles"); - const listComments = Symbol("listComments"); - const checkRuns = []; - const createdComments = []; - const updatedComments = []; - const deletedCommentIds = []; - - const github = { - paginate: async (endpoint, _opts) => { - if (endpoint === listReviews) return reviews; - if (endpoint === listFiles) return files; - if (endpoint === listComments) return existingComments; - return []; - }, - rest: { - pulls: { - listReviews, - listFiles, - }, - checks: { - create: async (params) => { - checkRuns.push(params); - }, - }, - issues: { - listComments, - deleteComment: async (params) => { - deletedCommentIds.push(params.comment_id); - }, - createComment: async (params) => { - createdComments.push(params); - }, - updateComment: async (params) => { - updatedComments.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; - }, - }, - }, - _checkRuns: checkRuns, - _comments: createdComments, - _updatedComments: updatedComments, - _deletedCommentIds: deletedCommentIds, - }; - return github; -} - -// --- Tests --- - -describe("maintainer-approval", () => { - let tmpDir; - let originalWorkspace; - - before(() => { - originalWorkspace = process.env.GITHUB_WORKSPACE; - tmpDir = makeTmpOwners(OWNERS_CONTENT, OWNERTEAMS_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, no comment", 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._checkRuns.length, 1); - assert.equal(github._checkRuns[0].conclusion, "success"); - assert.ok(github._checkRuns[0].output.summary.includes("maintainer1")); - assert.equal(github._comments.length, 0); - assert.equal(github._updatedComments.length, 0); - }); - - it("approval cleans up stale pending comment", async () => { - const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "maintainer1" } }, - ], - files: [{ filename: "cmd/pipelines/foo.go" }], - existingComments: [ - { id: 500, body: "\n## Waiting for approval\n..." }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._checkRuns[0].conclusion, "success"); - assert.deepEqual(github._deletedCommentIds, [500]); - assert.equal(github._comments.length, 0); - assert.equal(github._updatedComments.length, 0); - }); - - it("maintainer-authored PR with any approval -> success, no comment", 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._checkRuns.length, 1); - assert.equal(github._checkRuns[0].conclusion, "success"); - assert.ok(github._checkRuns[0].output.summary.includes("maintainer-authored")); - assert.equal(github._comments.length, 0); - assert.equal(github._updatedComments.length, 0); - }); - - it("single domain, owner approved -> success, no comment", 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._checkRuns.length, 1); - assert.equal(github._checkRuns[0].conclusion, "success"); - assert.equal(github._comments.length, 0); - assert.equal(github._updatedComments.length, 0); - }); - - it("cross-domain, both approved -> success, no comment", 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._checkRuns.length, 1); - assert.equal(github._checkRuns[0].conclusion, "success"); - assert.equal(github._comments.length, 0); - assert.equal(github._updatedComments.length, 0); - }); - - 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 }); - - // No check run created; the required check stays as "Expected" (yellow dot). - assert.equal(github._checkRuns.length, 0); - }); - - it("wildcard files present -> pending, no check run", 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._checkRuns.length, 0); - }); - - it("no approvals at all -> pending, no check run", 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._checkRuns.length, 0); - }); - - it("OWNERTEAMS member approved -> success for team-owned path", async () => { - const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "teamdev1" } }, - ], - files: [{ filename: "cmd/apps/main.go" }], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].conclusion, "success"); - }); - - it("non-OWNERTEAMS-member approval for team-owned path -> pending", async () => { - const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "outsider" } }, - ], - files: [{ filename: "cmd/apps/main.go" }], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._checkRuns.length, 0); - }); - - 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._checkRuns.length, 0); - }); - - 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._checkRuns.length, 0); - }); - - 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 }); - }); - - // --- Comment upsert tests --- - - it("creates comment with MARKER when none exists", 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._comments.length, 1); - assert.ok(github._comments[0].body.includes("")); - assert.equal(github._updatedComments.length, 0); - assert.equal(github._deletedCommentIds.length, 0); - }); - - it("edits existing comment in place when body changes", async () => { - const github = makeGithub({ - reviews: [], - files: [{ filename: "cmd/pipelines/foo.go" }], - existingComments: [ - { id: 999, body: "\nOld comment" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._updatedComments.length, 1); - assert.equal(github._updatedComments[0].comment_id, 999); - assert.ok(github._updatedComments[0].body.includes("")); - assert.equal(github._comments.length, 0); - assert.equal(github._deletedCommentIds.length, 0); - }); - - it("skips edit when comment body is unchanged", async () => { - // Stub Math.random so selectRoundRobin is deterministic across runs. - const origRandom = Math.random; - Math.random = () => 0.5; - try { - // First, run once to capture the comment body. - const github1 = makeGithub({ - reviews: [], - files: [{ filename: "cmd/pipelines/foo.go" }], - }); - const core1 = makeCore(); - await runModule({ github: github1, context: makeContext(), core: core1 }); - const expectedBody = github1._comments[0].body; - - // Second run with that body as the existing comment. - const github2 = makeGithub({ - reviews: [], - files: [{ filename: "cmd/pipelines/foo.go" }], - existingComments: [ - { id: 100, body: expectedBody }, - ], - }); - const core2 = makeCore(); - await runModule({ github: github2, context: makeContext(), core: core2 }); - - assert.equal(github2._comments.length, 0); - assert.equal(github2._updatedComments.length, 0); - assert.equal(github2._deletedCommentIds.length, 0); - } finally { - Math.random = origRandom; - } - }); - - it("cleans up duplicate marker comments, keeps the first", async () => { - const github = makeGithub({ - reviews: [], - files: [{ filename: "cmd/pipelines/foo.go" }], - existingComments: [ - { id: 100, body: "\nFirst" }, - { id: 200, body: "\nDuplicate" }, - { id: 300, body: "\nLegacy" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - // Duplicates deleted, first one edited. - assert.deepEqual(github._deletedCommentIds.sort(), [200, 300]); - assert.equal(github._updatedComments.length, 1); - assert.equal(github._updatedComments[0].comment_id, 100); - assert.equal(github._comments.length, 0); - }); - - it("does not touch comments without the marker", async () => { - const github = makeGithub({ - reviews: [], - files: [{ filename: "cmd/pipelines/foo.go" }], - existingComments: [ - { id: 888, body: "Some unrelated comment" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._deletedCommentIds.length, 0); - assert.equal(github._updatedComments.length, 0); - assert.equal(github._comments.length, 1); - }); - - it("pending single-domain comment includes waiting header", 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._comments.length, 1); - assert.ok(github._comments[0].body.includes("## Waiting for approval")); - }); - - it("pending cross-domain comment includes per-group sections", 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._comments.length, 1); - const body = github._comments[0].body; - assert.ok(body.includes("## Approval status: pending")); - assert.ok(body.includes("`/cmd/pipelines/`")); - assert.ok(body.includes("`/bundle/`")); - assert.ok(body.includes("approved by `@jefferycheng1`")); - assert.ok(body.includes("needs approval")); - }); - - it("lists individual files when fewer than 4 in a group", async () => { - const github = makeGithub({ - reviews: [], - files: [ - { filename: "cmd/pipelines/foo.go" }, - { filename: "bundle/config.go" }, - { filename: "bundle/deploy.go" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._comments.length, 1); - const body = github._comments[0].body; - assert.ok(body.includes("Files:"), "should list individual files"); - assert.ok(body.includes("`bundle/config.go`")); - assert.ok(body.includes("`bundle/deploy.go`")); - }); - - it("shows file count instead of listing when 4 or more files in a group", async () => { - const github = makeGithub({ - reviews: [], - files: [ - { filename: "cmd/pipelines/foo.go" }, - { filename: "bundle/a.go" }, - { filename: "bundle/b.go" }, - { filename: "bundle/c.go" }, - { filename: "bundle/d.go" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._comments.length, 1); - const body = github._comments[0].body; - assert.ok(body.includes("4 files changed"), "should show count for bundle group"); - assert.ok(!body.includes("`bundle/a.go`"), "should not list individual bundle files"); - }); -}); diff --git a/.github/workflows/maintainer-approval.yml b/.github/workflows/maintainer-approval.yml deleted file mode 100644 index 6bf1dad74ee..00000000000 --- a/.github/workflows/maintainer-approval.yml +++ /dev/null @@ -1,70 +0,0 @@ -name: PR approval - -on: - pull_request_target: - types: [opened, synchronize, reopened, ready_for_review] - pull_request_review: - types: [submitted, dismissed] - merge_group: - types: [checks_requested] - -# No concurrency group: a run cancelled by concurrency leaves a stuck failing -# check, since each run posts its own check-run and a later passing run does not -# clear it. The check is cheap and idempotent, so let every run finish instead. - -defaults: - run: - shell: bash - -jobs: - # Auto-approve maintainer-approval for merge queue entries. - # PRs are already approved before entering the merge queue, - # so we just need to set the status on the merge queue commit. - merge-queue-approval: - if: ${{ github.event_name == 'merge_group' }} - runs-on: - group: databricks-deco-testing-runner-group - labels: ubuntu-latest-deco - permissions: - checks: write - steps: - - name: Auto-approve for merge queue - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - with: - script: | - await github.rest.checks.create({ - owner: context.repo.owner, - repo: context.repo.repo, - head_sha: context.sha, - name: 'maintainer-approval', - status: 'completed', - conclusion: 'success', - output: { - title: 'maintainer-approval', - summary: 'Auto-approved (merge queue)', - }, - }); - - check: - runs-on: - group: databricks-deco-testing-runner-group - labels: ubuntu-latest-deco - if: ${{ github.event_name != 'merge_group' && !github.event.pull_request.draft }} - timeout-minutes: 5 - permissions: - pull-requests: write - checks: write - contents: read - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - ref: ${{ github.event.pull_request.base.sha }} - persist-credentials: false - fetch-depth: 0 - - name: Check approval and suggest reviewers - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - with: - retries: 3 - script: |- - const script = require('./.github/workflows/maintainer-approval.js'); - await script({ context, github, core }); diff --git a/.github/workflows/test-owners-scripts.yml b/.github/workflows/test-owners-scripts.yml deleted file mode 100644 index cbb56c56783..00000000000 --- a/.github/workflows/test-owners-scripts.yml +++ /dev/null @@ -1,26 +0,0 @@ -name: Test OWNERS scripts - -on: - pull_request: - paths: - - '.github/scripts/**' - - '.github/workflows/maintainer-approval.js' - - '.github/workflows/maintainer-approval.test.js' - - '.github/workflows/test-owners-scripts.yml' - - '.github/OWNERS' - - '.github/OWNERTEAMS' - -jobs: - test: - runs-on: - group: databricks-deco-testing-runner-group - labels: ubuntu-latest-deco - timeout-minutes: 5 - steps: - # Full checkout (no sparse-checkout): `owners.js validate` verifies that - # every OWNERS path exists in the tree, so it needs the whole repo. - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - name: Run OWNERS script tests - run: node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js - - name: Validate OWNERS and OWNERTEAMS - run: node .github/scripts/owners.js validate diff --git a/Taskfile.yml b/Taskfile.yml index 2ec9e6220ca..5c1919f572b 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -286,6 +286,11 @@ tasks: cmds: - "./tools/validate_nextchanges.py" + check-codeowners: + desc: Check that CLI maintainers are the first owner on every CODEOWNERS rule + cmds: + - 'uv run -p ">=3.11" --no-project python tools/validate_codeowners.py' + changelog-preview: desc: Print the CHANGELOG.md section the next release would add from .nextchanges/ env: @@ -319,7 +324,7 @@ tasks: - "./tools/validate_agents_setup.py" checks: - desc: Run quick checks (tidy, whitespace, deadcode, changelog, lockfiles, agent setup) + desc: Run quick checks (tidy, whitespace, deadcode, changelog, codeowners, lockfiles, agent setup) # Sequential: `tidy` rewrites go.mod/go.sum and any future tidy work # touching more paths should not race with the whitespace scanner. cmds: @@ -327,6 +332,7 @@ tasks: - task: ws - task: deadcode - task: check-changelog + - task: check-codeowners - task: check-lockfiles - task: check-agents-setup diff --git a/tools/validate_codeowners.py b/tools/validate_codeowners.py new file mode 100644 index 00000000000..4404a190457 --- /dev/null +++ b/tools/validate_codeowners.py @@ -0,0 +1,79 @@ +#!/usr/bin/env python3 +"""Keep CLI maintainers eligible to approve every CODEOWNERS rule.""" + +import sys +from pathlib import Path + +MAINTAINERS = "@databricks/eng-deco-cli" + + +def validate_contents(data): + r"""Return line-numbered errors for missing or misplaced maintainers. + + A maintainer catch-all followed by area owners is valid: + + >>> catch_all = '* @databricks/eng-deco-cli\n' + >>> validate_contents(catch_all + '/bundle/ @databricks/eng-deco-cli @databricks/eng-deco-dabs\n') + [] + + Ignore blank lines and comments, including inline comments: + + >>> validate_contents('\n # Owners\n\t*\t@databricks/eng-deco-cli # Default\r\n') + [] + + Missing owners, a different first owner, and a maintainer listed later fail: + + >>> validate_contents(catch_all + '/bundle/\n') + ['2: first owner must be @databricks/eng-deco-cli'] + >>> validate_contents(catch_all + '/bundle/ @databricks/eng-deco-dabs\n') + ['2: first owner must be @databricks/eng-deco-cli'] + >>> validate_contents(catch_all + '/bundle/ @databricks/eng-deco-dabs @databricks/eng-deco-cli\n') + ['2: first owner must be @databricks/eng-deco-cli'] + + A comment or similarly named team cannot satisfy the owner requirement: + + >>> validate_contents(catch_all + '/bundle/ # @databricks/eng-deco-cli\n') + ['2: first owner must be @databricks/eng-deco-cli'] + >>> validate_contents('* @databricks/eng-deco-cli-extra\n') + ['1: first owner must be @databricks/eng-deco-cli'] + + The first rule must cover all paths, even if a catch-all appears later: + + >>> validate_contents('# Owners\n/bundle/ @databricks/eng-deco-cli\n' + catch_all) + ["2: first rule must use '*' to cover all paths"] + >>> validate_contents('') + ["1: missing '* @databricks/eng-deco-cli' catch-all rule"] + >>> validate_contents('\n# No rules\n') + ["1: missing '* @databricks/eng-deco-cli' catch-all rule"] + + Report every invalid rule with its actual line number: + + >>> validate_contents(catch_all + '\n# Bundles\n/bundle/\n/cmd/bundle/ @databricks/eng-deco-dabs\n') + ['4: first owner must be @databricks/eng-deco-cli', '5: first owner must be @databricks/eng-deco-cli'] + """ + errors = [] + found_rule = False + for lineno, line in enumerate(data.splitlines(), 1): + fields = line.split() + if not fields or fields[0].startswith("#"): + continue + if not found_rule and fields[0] != "*": + errors.append(f"{lineno}: first rule must use '*' to cover all paths") + found_rule = True + if fields[1:2] != [MAINTAINERS]: + errors.append(f"{lineno}: first owner must be {MAINTAINERS}") + if not found_rule: + errors.append(f"1: missing '* {MAINTAINERS}' catch-all rule") + return errors + + +def main(): + path = Path(".github/CODEOWNERS") + errors = validate_contents(path.read_text(encoding="utf-8")) + for error in errors: + print(f"{path}:{error}") + return 1 if errors else 0 + + +if __name__ == "__main__": + sys.exit(main())