-
Notifications
You must be signed in to change notification settings - Fork 250
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
feat_: implement PR review workflow #5877
base: develop
Are you sure you want to change the base?
Changes from 1 commit
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,145 @@ | ||||||||||||||||||||
name: PR Review Workflow for Breaking Changes | ||||||||||||||||||||
|
||||||||||||||||||||
on: | ||||||||||||||||||||
pull_request: | ||||||||||||||||||||
types: | ||||||||||||||||||||
- opened | ||||||||||||||||||||
- reopened | ||||||||||||||||||||
- labeled | ||||||||||||||||||||
- unlabeled | ||||||||||||||||||||
- synchronize | ||||||||||||||||||||
|
||||||||||||||||||||
jobs: | ||||||||||||||||||||
check_breaking_change: | ||||||||||||||||||||
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. |
||||||||||||||||||||
runs-on: ubuntu-latest | ||||||||||||||||||||
steps: | ||||||||||||||||||||
- name: Check for breaking change label | ||||||||||||||||||||
id: check_label | ||||||||||||||||||||
uses: actions/github-script@v6 | ||||||||||||||||||||
with: | ||||||||||||||||||||
github-token: ${{secrets.GITHUB_TOKEN}} | ||||||||||||||||||||
script: | | ||||||||||||||||||||
const { data: labels } = await github.rest.issues.listLabelsOnIssue({ | ||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
issue_number: context.issue.number, | ||||||||||||||||||||
}); | ||||||||||||||||||||
const hasBreakingChange = labels.some(label => label.name === 'breaking change'); | ||||||||||||||||||||
console.log(`Has breaking change label: ${hasBreakingChange}`); | ||||||||||||||||||||
return hasBreakingChange; | ||||||||||||||||||||
|
||||||||||||||||||||
- name: Set QA reviewers | ||||||||||||||||||||
if: steps.check_label.outputs.result == 'true' | ||||||||||||||||||||
id: set_reviewers | ||||||||||||||||||||
run: | | ||||||||||||||||||||
echo "mobile_qa=churik,yevh-berdnyk,VolodLytvynenko,pavloburykh,mariia-skrypnyk,Horupa-Olena" >> $GITHUB_OUTPUT | ||||||||||||||||||||
echo "desktop_qa=anastasiyaig,virginiabalducci,glitchminer,antdanchenko" >> $GITHUB_OUTPUT | ||||||||||||||||||||
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. For the time being I'm hardcoding desktop and mobile QA members here. 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. @igor-sirotin : we can also write some logic to randomise who gets requested for review since we now decide who to request a review ;) 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. This works ofc, if it's simpler 👍 We can improve in future. Though I'm worried about pinging all QAs, we end up with the list like this: Imagine what will happen when we add devs reviews as well 🥲 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. Then can't we just put that in a separate file? 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. separate file indeed looks a bit easier to manage. |
||||||||||||||||||||
|
||||||||||||||||||||
- name: Request QA reviews | ||||||||||||||||||||
if: steps.check_label.outputs.result == 'true' | ||||||||||||||||||||
uses: actions/github-script@v6 | ||||||||||||||||||||
with: | ||||||||||||||||||||
github-token: ${{secrets.GITHUB_TOKEN}} | ||||||||||||||||||||
script: | | ||||||||||||||||||||
const mobileQA = '${{ steps.set_reviewers.outputs.mobile_qa }}'.split(','); | ||||||||||||||||||||
const desktopQA = '${{ steps.set_reviewers.outputs.desktop_qa }}'.split(','); | ||||||||||||||||||||
const reviewers = [...mobileQA, ...desktopQA]; | ||||||||||||||||||||
|
||||||||||||||||||||
await github.rest.pulls.requestReviewers({ | ||||||||||||||||||||
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. Might this re-request reviews, if already approved? I think we don't want such behaviour? |
||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
pull_number: context.issue.number, | ||||||||||||||||||||
reviewers: reviewers | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
- name: Check QA approvals | ||||||||||||||||||||
if: steps.check_label.outputs.result == 'true' | ||||||||||||||||||||
id: check_approvals | ||||||||||||||||||||
uses: actions/github-script@v6 | ||||||||||||||||||||
with: | ||||||||||||||||||||
github-token: ${{secrets.GITHUB_TOKEN}} | ||||||||||||||||||||
script: | | ||||||||||||||||||||
const { data: reviews } = await github.rest.pulls.listReviews({ | ||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
pull_number: context.issue.number, | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
const mobileQA = '${{ steps.set_reviewers.outputs.mobile_qa }}'.split(','); | ||||||||||||||||||||
const desktopQA = '${{ steps.set_reviewers.outputs.desktop_qa }}'.split(','); | ||||||||||||||||||||
|
||||||||||||||||||||
const mobileQAApproved = reviews.some(review => | ||||||||||||||||||||
review.state === 'APPROVED' && mobileQA.includes(review.user.login) | ||||||||||||||||||||
); | ||||||||||||||||||||
const desktopQAApproved = reviews.some(review => | ||||||||||||||||||||
review.state === 'APPROVED' && desktopQA.includes(review.user.login) | ||||||||||||||||||||
); | ||||||||||||||||||||
|
||||||||||||||||||||
console.log(`Mobile QA approved: ${mobileQAApproved}`); | ||||||||||||||||||||
console.log(`Desktop QA approved: ${desktopQAApproved}`); | ||||||||||||||||||||
|
||||||||||||||||||||
core.setOutput('mobile-qa-approved', mobileQAApproved); | ||||||||||||||||||||
core.setOutput('desktop-qa-approved', desktopQAApproved); | ||||||||||||||||||||
|
||||||||||||||||||||
return mobileQAApproved && desktopQAApproved; | ||||||||||||||||||||
|
||||||||||||||||||||
- name: Block PR if conditions not met | ||||||||||||||||||||
if: steps.check_label.outputs.result == 'true' && steps.check_approvals.outputs.result != 'true' | ||||||||||||||||||||
uses: actions/github-script@v6 | ||||||||||||||||||||
with: | ||||||||||||||||||||
github-token: ${{secrets.GITHUB_TOKEN}} | ||||||||||||||||||||
script: | | ||||||||||||||||||||
const mobileQAApproved = ${{ steps.check_approvals.outputs.mobile-qa-approved }}; | ||||||||||||||||||||
const desktopQAApproved = ${{ steps.check_approvals.outputs.desktop-qa-approved }}; | ||||||||||||||||||||
|
||||||||||||||||||||
let message = 'This PR has the breaking change label and requires approval from both mobile-qa and desktop-qa teams before it can be merged.\n\n'; | ||||||||||||||||||||
|
||||||||||||||||||||
if (!mobileQAApproved && !desktopQAApproved) { | ||||||||||||||||||||
message += 'Both mobile-qa and desktop-qa teams have not approved this PR yet.'; | ||||||||||||||||||||
} else if (!mobileQAApproved) { | ||||||||||||||||||||
message += 'The mobile-qa team has not approved this PR yet.'; | ||||||||||||||||||||
} else if (!desktopQAApproved) { | ||||||||||||||||||||
message += 'The desktop-qa team has not approved this PR yet.'; | ||||||||||||||||||||
} | ||||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
await github.rest.checks.create({ | ||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
name: 'QA Approval Check', | ||||||||||||||||||||
head_sha: context.payload.pull_request.head.sha, | ||||||||||||||||||||
status: 'completed', | ||||||||||||||||||||
conclusion: 'failure', | ||||||||||||||||||||
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. @igor-sirotin : added a failing check as discussed, the github-actions bot will no longer request changes, It will just add a comment with an appropriate message. 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. Does this create a separate status check? Maybe we could just use this check itself? status-go/.github/workflows/commit-check.yml Line 100 in a1c6d7f
The reason is not to bloat the checks list. It's already 12 😅 And the comment can be implemented as "sticky comment", I did it in the |
||||||||||||||||||||
output: { | ||||||||||||||||||||
title: 'QA Approval Required', | ||||||||||||||||||||
summary: message | ||||||||||||||||||||
} | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
- name: Allow PR merge if conditions are met | ||||||||||||||||||||
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. Do we need the bot approval at all, if we have the status check already? It's kinda duplication 🤔 |
||||||||||||||||||||
if: steps.check_label.outputs.result == 'true' && steps.check_approvals.outputs.result == 'true' | ||||||||||||||||||||
uses: actions/github-script@v6 | ||||||||||||||||||||
with: | ||||||||||||||||||||
github-token: ${{secrets.GITHUB_TOKEN}} | ||||||||||||||||||||
script: | | ||||||||||||||||||||
|
||||||||||||||||||||
const { data: reviews } = await github.rest.pulls.listReviews({ | ||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
pull_number: context.issue.number, | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
||||||||||||||||||||
const botApprovalExists = reviews.some(review => | ||||||||||||||||||||
review.user.login === 'github-actions[bot]' && review.state === 'APPROVED' | ||||||||||||||||||||
); | ||||||||||||||||||||
|
||||||||||||||||||||
if (!botApprovalExists) { | ||||||||||||||||||||
await github.rest.pulls.createReview({ | ||||||||||||||||||||
owner: context.repo.owner, | ||||||||||||||||||||
repo: context.repo.repo, | ||||||||||||||||||||
pull_number: context.issue.number, | ||||||||||||||||||||
event: 'APPROVE', | ||||||||||||||||||||
body: 'Breaking changes have been approved by Mobile and Desktop QA. This PR can now be merged.' | ||||||||||||||||||||
}); | ||||||||||||||||||||
} else { | ||||||||||||||||||||
console.log('Bot approval already exists. No action taken.'); | ||||||||||||||||||||
} |
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.
Shorter name would be better I think