-
Notifications
You must be signed in to change notification settings - Fork 18
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
Modify commitee TOML and workflow #67
Changes from 16 commits
0568983
2c323e1
a25a449
d31df34
aae2423
ceb21e7
3f530a4
0fd93bf
233661d
22e770b
1f6eceb
ff9ff58
5ac231e
257f8bb
6cac23c
b079d37
efc68d0
9557879
d3c3e51
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 |
---|---|---|
|
@@ -10,5 +10,6 @@ members = [ | |
"qinheping", | ||
"tautschnig", | ||
"jaisnan", | ||
"patricklam" | ||
"patricklam", | ||
"carolynzech", | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,14 @@ name: Check PR Approvals | |
# To protect against that scenario, we can turn on number of approvals required to 2 in the github settings | ||
# of the repository | ||
on: | ||
workflow_dispatch: | ||
pull_request: | ||
branches: [ main ] | ||
Comment on lines
+9
to
+10
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 you need this? |
||
pull_request_review: | ||
types: [submitted] | ||
workflow_dispatch: | ||
|
||
# Without these permissions, we get a 403 error from github | ||
# for trying to modify the pull request for newer project. | ||
# Source: https://stackoverflow.com/a/76994510 | ||
permissions: write-all | ||
|
||
jobs: | ||
check-approvals: | ||
if: github.event.review.state == 'APPROVED' || github.event_name == 'workflow_dispatch' | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout repository | ||
|
@@ -58,7 +54,7 @@ jobs: | |
pull_number | ||
}); | ||
|
||
const relevantPaths = ['library/', 'doc/src/challenges/']; | ||
const relevantPaths = ['library/', 'doc/src/challenges/', '.github/workflows']; | ||
const isRelevantPR = files.data.some(file => | ||
relevantPaths.some(path => file.filename.startsWith(path)) | ||
); | ||
|
@@ -73,7 +69,6 @@ jobs: | |
const tomlContent = fs.readFileSync('.github/pull_requests.toml', 'utf8'); | ||
console.log('TOML content:', tomlContent); | ||
const tomlData = toml.parse(tomlContent); | ||
console.log('Parsed TOML data:', JSON.stringify(tomlData, null, 2)); | ||
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. Have you considered the business logic to a script instead of hard coding it in here? I think it is usually much easier to debug and run it locally. For example, you could create a script that takes as an argument a list of logins and the script succeeds if and only if the minimum number of people in the committee has been achieved. Or the script output the number of people from the committee from that list and you only check here if the number is >= minimum requirement. 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. Thank you for that idea! I shall move the logic to its own script ad use gh command line to try and rerun old failing checks. |
||
|
||
if (!tomlData.committee || !Array.isArray(tomlData.committee.members)) { | ||
throw new Error('committee.members is not an array in the TOML file'); | ||
|
@@ -110,52 +105,16 @@ jobs: | |
|
||
// Core logic that checks if the approvers are in the committee | ||
const checkName = 'PR Approval Status'; | ||
const conclusion = (approvers.size >= requiredApprovals && currentCountfromCommittee >= 2) ? 'success' : 'failure'; | ||
const conclusion = (currentCountfromCommittee >= requiredApprovals) ? 'success' : 'failure'; | ||
const output = { | ||
title: checkName, | ||
summary: `PR has ${approvers.size} total approvals and ${requiredApprovals} required approvals.`, | ||
text: `Approvers: ${Array.from(approvers).join(', ')}\nRequired Approvers: ${requiredApprovers.join(', ')}` | ||
}; | ||
|
||
// Get PR details | ||
const pr = await github.rest.pulls.get({ | ||
owner, | ||
repo, | ||
pull_number | ||
}); | ||
|
||
// Create or update check run | ||
const checkRuns = await github.rest.checks.listForRef({ | ||
owner, | ||
repo, | ||
ref: pr.data.head.sha, | ||
check_name: checkName | ||
}); | ||
|
||
// Reuse the same workflow everytime there's a new review submitted | ||
// instead of creating new workflows. Better efficiency and readability | ||
// as the number of workflows is kept to a minimal number | ||
if (checkRuns.data.total_count > 0) { | ||
await github.rest.checks.update({ | ||
owner, | ||
repo, | ||
check_run_id: checkRuns.data.check_runs[0].id, | ||
status: 'completed', | ||
conclusion, | ||
output | ||
}); | ||
} else { | ||
await github.rest.checks.create({ | ||
owner, | ||
repo, | ||
name: checkName, | ||
head_sha: pr.data.head.sha, | ||
status: 'completed', | ||
conclusion, | ||
output | ||
}); | ||
} | ||
|
||
// Simplify to not use github create at all | ||
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 was this seemingly necessary before, but turns it it actually isn't? |
||
if (conclusion === 'failure') { | ||
core.setFailed(`PR needs at least ${requiredApprovals} total approvals and 2 required approvals. Current approvals: ${approvers.size}, Required approvals: ${requiredApprovals}`); | ||
} else { | ||
console.log(`PR has sufficient approvals (${approvers.size}/${requiredApprovals})`); | ||
} |
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.
This doesn't run in the context of a PR though, does it? You could add an option to provide a PR link or a PR number. But I don't know if it's worth it