-
Notifications
You must be signed in to change notification settings - Fork 896
Customize codeowner approval logic as a status check workflow #2542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,90 @@ | ||||||||||||||||||||||||||||||
| name: Code-owner Approval Status | ||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||
| pull_request_review: | ||||||||||||||||||||||||||||||
| types: [submitted] | ||||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||||
| types: [opened, synchronize] | ||||||||||||||||||||||||||||||
|
Comment on lines
+2
to
+6
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Workflow cannot set statuses on fork PRs.
Proposed fix on:
pull_request_review:
types: [submitted]
- pull_request:
+ pull_request_target:
types: [opened, synchronize]Note: π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||||||||
| pull-requests: read | ||||||||||||||||||||||||||||||
| statuses: write | ||||||||||||||||||||||||||||||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing When the Proposed fix permissions:
+ contents: read
pull-requests: read
statuses: writeπ Committable suggestion
Suggested change
π€ Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do i need to clone repo? codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8') seems to be working in an experimental repo
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π§© Analysis chainπ Script executed: Length of output: 3695
When you explicitly define a
If your experimental repo worked without
You can verify this by checking the Actions logsβwithout TL;DR: |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||||||
| check: | ||||||||||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||
| - uses: actions/checkout@v4 | ||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||
| ref: ${{ github.event.pull_request.base.sha }} | ||||||||||||||||||||||||||||||
| - run: npm install minimatch@9 | ||||||||||||||||||||||||||||||
| - uses: actions/github-script@v7 | ||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||||||
| const files = await github.paginate(github.rest.pulls.listFiles, { | ||||||||||||||||||||||||||||||
| owner: context.repo.owner, repo: context.repo.repo, | ||||||||||||||||||||||||||||||
| pull_number: context.payload.pull_request.number, | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
| const reviews = await github.paginate(github.rest.pulls.listReviews, { | ||||||||||||||||||||||||||||||
| owner: context.repo.owner, repo: context.repo.repo, | ||||||||||||||||||||||||||||||
| pull_number: context.payload.pull_request.number, | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Parse CODEOWNERS (reuse your existing file) | ||||||||||||||||||||||||||||||
| const fs = require('fs'); | ||||||||||||||||||||||||||||||
| let codeowners = []; | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8') | ||||||||||||||||||||||||||||||
| .split('\n').filter(l => l.trim() && !l.startsWith('#')) | ||||||||||||||||||||||||||||||
| .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; }); | ||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lines with leading whitespace will be misparsed.
Proposed fix codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
- .split('\n').filter(l => l.trim() && !l.startsWith('#'))
- .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
+ .split('\n').map(l => l.trim()).filter(l => l && !l.startsWith('#'))
+ .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||||||||||
| // Do nothing, but let the same logic handle the error later. | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+35
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent catch produces a misleading failure message when CODEOWNERS is missing. If the file doesn't exist or can't be read, Also, Proposed fix try {
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
- .split('\n').filter(l => l.trim() && !l.startsWith('#'))
+ .split('\n').filter(l => l.trim() && !l.trim().startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
} catch (e) {
- // Do nothing, but let the same logic handle the error later.
+ core.warning(`Could not read .github/CODEOWNERS: ${e.message}`);
}π Committable suggestion
Suggested change
π€ Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. intentional for simplicity. The maintainer will check the file if it doesn't exist etc and discover the problem by debugging
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const { minimatch } = require('minimatch'); | ||||||||||||||||||||||||||||||
| const allOwners = new Set(); | ||||||||||||||||||||||||||||||
| for (const f of files) { | ||||||||||||||||||||||||||||||
| // Each file's code owner is determined by the last matched rule | ||||||||||||||||||||||||||||||
| // This a native CODEOWNERS file behavior | ||||||||||||||||||||||||||||||
| let lastMatch = null; | ||||||||||||||||||||||||||||||
| for (const rule of codeowners) { | ||||||||||||||||||||||||||||||
| if (minimatch(f.filename, rule.pattern)) lastMatch = rule; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if (lastMatch) lastMatch.owners.forEach(o => allOwners.add(o.replace('@', ''))); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+45
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
CODEOWNERS patterns follow Proposed fix- if (minimatch(f.filename, rule.pattern)) lastMatch = rule;
+ if (minimatch(f.filename, rule.pattern, { matchBase: true })) lastMatch = rule;π€ Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apply the change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Task: Add matchBase option to minimatch call
π‘ For code changes across multiple files, use |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Note allOwners can be an empty set and is accepted by the following logic. | ||||||||||||||||||||||||||||||
| // If empty, "failure" will be issued for this check, blocking the merge. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const prAuthor = context.payload.pull_request.user.login; | ||||||||||||||||||||||||||||||
| const hasQualifiedApprover = !(allOwners.size === 0 || (allOwners.size === 1 && allOwners.has(prAuthor))); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const latestReviews = new Map(); | ||||||||||||||||||||||||||||||
| for (const r of reviews) { | ||||||||||||||||||||||||||||||
| latestReviews.set(r.user.login, r); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const approved = hasQualifiedApprover && [...latestReviews.values()].some(r => | ||||||||||||||||||||||||||||||
| r.state === 'APPROVED' && allOwners.has(r.user.login) | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const owners_array = Array.from(allOwners); | ||||||||||||||||||||||||||||||
| const display = hasQualifiedApprover | ||||||||||||||||||||||||||||||
| ? (owners_array.length <= 3 | ||||||||||||||||||||||||||||||
| ? owners_array.map(o => `@${o}`).join(', ') | ||||||||||||||||||||||||||||||
| : `@${owners_array.slice(0, 3).join(', @')} +${owners_array.length - 3} more`) | ||||||||||||||||||||||||||||||
| : "β no qualified approver. update CODEOWNERS!"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const approver = approved | ||||||||||||||||||||||||||||||
| ? [...latestReviews.values()].find(r => r.state === 'APPROVED' && allOwners.has(r.user.login)).user.login | ||||||||||||||||||||||||||||||
| : null; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| await github.rest.repos.createCommitStatus({ | ||||||||||||||||||||||||||||||
| owner: context.repo.owner, repo: context.repo.repo, | ||||||||||||||||||||||||||||||
| sha: context.payload.pull_request.head.sha, | ||||||||||||||||||||||||||||||
| state: approved ? 'success' : 'failure', | ||||||||||||||||||||||||||||||
| context: 'codeowner-approval', | ||||||||||||||||||||||||||||||
| description: approved | ||||||||||||||||||||||||||||||
| ? `Approved by ${approver}` | ||||||||||||||||||||||||||||||
| : `β need approval: ${display}`, | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
Comment on lines
+81
to
+89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub commit-status If there are many owners with long usernames, Consider truncating explicitly and keeping the Proposed fix+ const descriptionRaw = approved
+ ? `Approved by ${approver}`
+ : hasQualifiedApprover
+ ? `Need approval from: ${owners_array.length <= 3 ? owners_array.map(o => `@${o}`).join(', ') : `@${owners_array.slice(0, 3).join(', @')} +${owners_array.length - 3} more`}`
+ : 'No qualified approver found β update CODEOWNERS';
+ const description = descriptionRaw.slice(0, 140);
+
await github.rest.repos.createCommitStatus({
owner: context.repo.owner, repo: context.repo.repo,
sha: context.payload.pull_request.head.sha,
state: approved ? 'success' : 'failure',
context: 'codeowner-approval',
- description: approved
- ? `Approved by ${approver}`
- : `β need approval: ${display}`,
+ description,
});π€ Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i already truncate this to top-3 in the code
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Two minor edge cases remain:
These are quite rare edge cases, so if you're comfortable with the current approach, it's reasonable to keep it as is. If you'd like to be extra defensive, adding an explicit π§ Learnings used |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix trailing whitespace β CI is failing. The pipeline reports a π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed reviews won't re-trigger this check.
If an approving review is dismissed, the commit status will remain
successuntil another event (like a new push) re-triggers the workflow. Adddismissedto thepull_request_reviewtypes to handle this case.Proposed fix
π Committable suggestion
π€ Prompt for AI Agents