From 28742fa628f9af398cfcf3b71dfc77da4cbbd617 Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Wed, 26 Jun 2024 16:23:44 +0000 Subject: [PATCH 1/7] Check for committee among the PR approval Add check only for library and challenges folder python error when checking for approvals replace with JS Add script to check for no of approvers Re-trigger same run, instead of creating new run Remove pull_request trigger Add workflow step Test re-trigger when second approval happens Optimize Add committe checking flow add second account to committee for testing Add comma Add try/catch block around the parsing change toml file name Change parsing fix name of toml file --- .github/pull_requests.toml | 6 ++ .github/workflows/pr_approval.yml | 147 ++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 .github/pull_requests.toml create mode 100644 .github/workflows/pr_approval.yml diff --git a/.github/pull_requests.toml b/.github/pull_requests.toml new file mode 100644 index 0000000000000..e663d04b4269d --- /dev/null +++ b/.github/pull_requests.toml @@ -0,0 +1,6 @@ +[committee] +members = ["celinval", +"jaisnan", +"jaisu-1", +"js96test-dev" +] diff --git a/.github/workflows/pr_approval.yml b/.github/workflows/pr_approval.yml new file mode 100644 index 0000000000000..4c3c09351dd51 --- /dev/null +++ b/.github/workflows/pr_approval.yml @@ -0,0 +1,147 @@ +name: Check PR Approvals + +on: + pull_request_review: + types: [submitted] + workflow_dispatch: # Allows manual triggering + +jobs: + check-approvals: + if: github.event.review.state == 'APPROVED' || github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v2 + + - name: Install TOML parser + run: npm install @iarna/toml + + - name: Check PR Relevance and Approvals + uses: actions/github-script@v6 + with: + github-token: ${{secrets.GITHUB_TOKEN}} + script: | + const fs = require('fs'); + const toml = require('@iarna/toml'); + const { owner, repo } = context.repo; + let pull_number; + + if (github.event_name === 'workflow_dispatch') { + const branch = github.ref.replace('refs/heads/', ''); + const prs = await github.rest.pulls.list({ + owner, + repo, + head: `${owner}:${branch}`, + state: 'open' + }); + if (prs.data.length === 0) { + console.log('No open PR found for this branch.'); + return; + } + pull_number = prs.data[0].number; + } else { + pull_number = context.issue.number; + } + + // Get PR files + const files = await github.rest.pulls.listFiles({ + owner, + repo, + pull_number + }); + + const relevantPaths = ['library/', 'doc/src/challenges/']; + const isRelevantPR = files.data.some(file => + relevantPaths.some(path => file.filename.startsWith(path)) + ); + + if (!isRelevantPR) { + console.log('PR does not touch relevant paths. Exiting workflow.'); + return; + } + + // Get parsed data + try { + 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)); + + if (!tomlData.committee || !Array.isArray(tomlData.committee.members)) { + throw new Error('committee.members is not an array in the TOML file'); + } + requiredApprovers = tomlData.committee.members; + } catch (error) { + console.error('Error reading or parsing TOML file:', error); + core.setFailed('Failed to read required approvers list'); + return; + } + + // Get all reviews + const reviews = await github.rest.pulls.listReviews({ + owner, + repo, + pull_number + }); + + const approvers = new Set( + reviews.data + .filter(review => review.state === 'APPROVED') + .map(review => review.user.login) + ); + + const requiredApprovals = 2; + const requiredApproversCount = Array.from(approvers) + .filter(approver => requiredApprovers.includes(approver)) + .length; + + console.log('PR Approvers:', Array.from(approvers)); + console.log('Required Approvers:', requiredApproversCount); + + const checkName = 'PR Approval Status'; + const conclusion = (approvers.size >= requiredApprovals && requiredApproversCount >= 2) ? 'success' : 'failure'; + const output = { + title: checkName, + summary: `PR has ${approvers.size} total approvals and ${requiredApproversCount} 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 + }); + + 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 + }); + } + + if (conclusion === 'failure') { + core.setFailed(`PR needs at least ${requiredApprovals} total approvals and 2 required approvals. Current approvals: ${approvers.size}, Required approvals: ${requiredApproversCount}`); + } From 4d3167759115433a6f538e874c0e6d5749a4f44d Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Mon, 1 Jul 2024 22:25:21 +0000 Subject: [PATCH 2/7] Fix user toml account --- .github/pull_requests.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/pull_requests.toml b/.github/pull_requests.toml index e663d04b4269d..eef2b58be3ded 100644 --- a/.github/pull_requests.toml +++ b/.github/pull_requests.toml @@ -1,6 +1,6 @@ [committee] members = ["celinval", "jaisnan", -"jaisu-1", +"Jaisu-1", "js96test-dev" ] From d7ce67d0809ecbc17755d0304dbaedfd98edb967 Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Mon, 1 Jul 2024 22:35:44 +0000 Subject: [PATCH 3/7] Remove account names --- .github/pull_requests.toml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/pull_requests.toml b/.github/pull_requests.toml index eef2b58be3ded..f8a64a981b1e8 100644 --- a/.github/pull_requests.toml +++ b/.github/pull_requests.toml @@ -1,6 +1,3 @@ [committee] -members = ["celinval", -"jaisnan", -"Jaisu-1", -"js96test-dev" +members = [ ] From ecfbc005b870bb54ced24349a9a0d37c5e58a96c Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Tue, 2 Jul 2024 15:35:34 +0000 Subject: [PATCH 4/7] Update comments --- .github/workflows/pr_approval.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr_approval.yml b/.github/workflows/pr_approval.yml index 4c3c09351dd51..c255840a0e2d7 100644 --- a/.github/workflows/pr_approval.yml +++ b/.github/workflows/pr_approval.yml @@ -1,9 +1,13 @@ name: Check PR Approvals +# For now, the workflow gets triggered only when a review is submitted +# This technically means, a PR with zero approvals can be merged by the rules of this workflow alone +# To protect against that scenario, we can turn on number of approvals required to 2 in the github settings +# of the repository on: pull_request_review: types: [submitted] - workflow_dispatch: # Allows manual triggering + workflow_dispatch: jobs: check-approvals: @@ -84,6 +88,7 @@ jobs: pull_number }); + # Example: approvers = ["celina", "zyad"] const approvers = new Set( reviews.data .filter(review => review.state === 'APPROVED') @@ -95,9 +100,11 @@ jobs: .filter(approver => requiredApprovers.includes(approver)) .length; + # TODO: Improve logging and messaging to the user console.log('PR Approvers:', Array.from(approvers)); console.log('Required Approvers:', requiredApproversCount); + # Core logic that checks if the approvers are in the committee const checkName = 'PR Approval Status'; const conclusion = (approvers.size >= requiredApprovals && requiredApproversCount >= 2) ? 'success' : 'failure'; const output = { @@ -121,6 +128,9 @@ jobs: 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, From 76a44b61450a077c3e2335a44228adac936f664a Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Tue, 2 Jul 2024 20:31:34 +0000 Subject: [PATCH 5/7] delete toml file from this PR as we're adding it in another PR first --- .github/pull_requests.toml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 .github/pull_requests.toml diff --git a/.github/pull_requests.toml b/.github/pull_requests.toml deleted file mode 100644 index f8a64a981b1e8..0000000000000 --- a/.github/pull_requests.toml +++ /dev/null @@ -1,3 +0,0 @@ -[committee] -members = [ -] From f4352a378a23219955aed9a852b6f0ee0a9c2d56 Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Tue, 9 Jul 2024 22:01:50 +0000 Subject: [PATCH 6/7] Remove token from script --- .github/workflows/pr_approval.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/pr_approval.yml b/.github/workflows/pr_approval.yml index c255840a0e2d7..36b4d3b15b439 100644 --- a/.github/workflows/pr_approval.yml +++ b/.github/workflows/pr_approval.yml @@ -23,7 +23,6 @@ jobs: - name: Check PR Relevance and Approvals uses: actions/github-script@v6 with: - github-token: ${{secrets.GITHUB_TOKEN}} script: | const fs = require('fs'); const toml = require('@iarna/toml'); From 937f74da351a3be4770f962b1e0b2e70833e2a18 Mon Sep 17 00:00:00 2001 From: Jaisurya Nanduri Date: Tue, 9 Jul 2024 22:18:38 +0000 Subject: [PATCH 7/7] Change comments --- .github/workflows/pr_approval.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr_approval.yml b/.github/workflows/pr_approval.yml index 36b4d3b15b439..316be83fc0e62 100644 --- a/.github/workflows/pr_approval.yml +++ b/.github/workflows/pr_approval.yml @@ -87,7 +87,7 @@ jobs: pull_number }); - # Example: approvers = ["celina", "zyad"] + // Example: approvers = ["celina", "zyad"] const approvers = new Set( reviews.data .filter(review => review.state === 'APPROVED') @@ -99,11 +99,11 @@ jobs: .filter(approver => requiredApprovers.includes(approver)) .length; - # TODO: Improve logging and messaging to the user + // TODO: Improve logging and messaging to the user console.log('PR Approvers:', Array.from(approvers)); console.log('Required Approvers:', requiredApproversCount); - # Core logic that checks if the approvers are in the committee + // Core logic that checks if the approvers are in the committee const checkName = 'PR Approval Status'; const conclusion = (approvers.size >= requiredApprovals && requiredApproversCount >= 2) ? 'success' : 'failure'; const output = { @@ -127,9 +127,9 @@ jobs: 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 + // 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,